Specification does not define variablesReference lifetime

[hoehrmann]

The specification does not define when variablesReference numbers become invalid and when previously used numbers can be used again without adverse or unexpected consequences.

[Enerccio]

well at least in my implementation variablesReference is unique number until stack frame changes, then it is something else.

[hoehrmann]

Related: microsoft/vscode#25652 (comment) - apparently debug adapters are supposed to have longer-lived variablesReference IDs - although that comment seems to be directly contradicted by microsoft/vscode#25652 (comment)

[MetalSlime0]

I wish variablesReference was just a string. With only 31 bits to work with and no lifetime information, I can't come up with any way to support variableReferences properly that doesn't just consume memory as the debugger is used without ever freeing it.

[weinand]

Yes, DAP does not specify the lifetime of a variablesReference.
That was an oversight.

Let's fix this...

Since DAP was inspired by the (now legacy) V8 debugger protocol, I've tried to find something related to "lifetime of object handles":

All objects exposed through the debugger is assigned an ID called a handle. This handle is serialized and can be used to identify objects. A handle has a certain lifetime after which it will no longer refer to the same object. Currently the lifetime of handles match the processing of a debug event. For each debug event handles are recycled.

This essentially describes what the DAP client "VS Code" assumes:
whenever a DAP "stopped" event is received, VS Code retrieves the stack frames for the location, and then the frame's scopes, and from the scopes the individual variables. While execution is stopped, all variablesReference received for that stack will remain valid (refer to the same variables). But as soon as execution resumes, the variablesReferences become invalid (and the DAP client should no longer use them). On the next "stopped" event, variablesReferences can reuse the previous values.

Instead of duplicating this "lifetime explanation" in lots of places in the DAP spec, I suggest that we create a new section in the DAP Overview, which can then be referenced from the DAP spec.

In addition we can add the following "recipe" for how to manage variablesReferences:

The simplest way to manage variablesReferences in a DA is by using sequentially assigned integer keys that map to the debugger objects representing variables. Whenever a new variableReference is needed for a variable object, a new key is created by incrementing a counter and then a corresponding association is added to the map.
When execution resumes the counter is set to 0 and the map is cleared.

/cc @roblourens @connor4312

[connor4312]

Lgtm

[MetalSlime0]

The counter solution seems insufficient to me when considering this:

microsoft/vscode#25652 (comment)

The expansion state of a Variable or Watch tree is preserved across steps if the variablesReference (https://github.com/Microsoft/vscode-debugadapter-node/blob/master/protocol/src/debugProtocol.ts#L1177) doesn't change.

It seems like any solution involving a simple counter would break this requirement whenever a step adds or removes a local variable, causing expanded variables to collapse, which would be really frustrating. It also isn't clear to me whether it would work well with the watch window when, say, the number of items in an array changes.

[weinand]

• Yes, as an implementation detail VS Code tries to remember the expansion state by associating it with variablesReference. This seems to work well in practice.
• the use of variablesReference in the "Watches" view does not differ from the "Variables" view, so the observed behavior is identical.

If there is a need to make expansion state management more robust, then a new DAP feature would be necessary. E.g. an optional "UUID" hint on DAP's Variable type. But I'm not sure whether debug adapters could actually provide that UUID easily.

[MetalSlime0]

This seems to work well in practice.

How does it work given the issues I described? Like, if I have two arrays expanded, and after stepping an item is added to array 1, and the contents of array 2 are fetched after array 1, how does an incrementing strategy not change all the variablesReference values in array 2, causing elements within it to collapse? Maybe I'm misunderstanding something. Could you point me to a language implementing this strategy that I could look at the DAP implementation for and also test for myself in VSCode?

If there is a need to make expansion state management more robust, then a new DAP feature would be necessary.

Well, yes. From my perspective it would be sufficient for the debugger to send a message saying that it no longer needs the contents of a variablesReference when all uses of it are collapsed by the user. I will admit this is a quick thought and there may be subtleties I'm missing.

[puremourning]

FWIW in vimspector, we don't assume that variablesRefernce is persistent and still maintain the expansion state of variables; You can actually persist/compare the 'name' from the variables response (assuming you have state to associate the request with the parent).

I don't think new UUID is completely necessary, unless you think that the above doesn't work (I've never seen it go wrong)

  def _ConsumeVariables( self, draw, parent, message ):
    new_variables = []
    for variable_body in message[ 'body' ][ 'variables' ]:
      if parent.variables is None:
        parent.variables = []

      # Find the variable in parent
      found = False
      for index, v in enumerate( parent.variables ):
        if v.variable[ 'name' ] == variable_body[ 'name' ]:
          variable = v
          found = True
          break
      if not found:
        variable = Variable( parent, variable_body )
      else:
        variable.Update( variable_body )

[MetalSlime0]

Yeah, the approach that sort of works seems to be to associate objects with indices in the debugee and accept that they won't be cleaned up until the objects themselves are destroyed.

The actual problem I'm having in practice is that I don't have a literal object for every thing I want to represent as an expandable thing in the debugger, and I'm worried about the performance impact of introducing one. For instance, an object for every call frame (to store the local variable indices) adds overhead to each call. So for things like that I try to represent the "object" as a combination of a thread ID and call frame index, which I can squeeze into 31 bits well enough, but then when I wanted to add another layer to that, 31 bits was insufficient. So I have to go the route of hurting performance just because I can't store enough information in a variablesReference. One solution to that is to make it easier for me to associate variablesReferences with my own data, which leaks memory if there's no cleanup when a variablesReference will never be used again. A different solution, and maybe easier, would be to make variablesReference allowed to be a string, so I can just store whatever I want in there.