Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify object lifetime in evaluate/output #462

Merged
merged 2 commits into from
Feb 14, 2024
Merged

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Feb 14, 2024

Closes #459

@connor4312 connor4312 enabled auto-merge (squash) February 14, 2024 00:16
@vscodenpa vscodenpa added this to the February 2024 milestone Feb 14, 2024
roblourens
roblourens previously approved these changes Feb 14, 2024
@connor4312 connor4312 merged commit 03ee800 into main Feb 14, 2024
2 checks passed
@connor4312 connor4312 deleted the connor4312/issue-459 branch February 14, 2024 17:08
@puremourning
Copy link
Contributor

I struggle to see how this clarification really clarifies anything. Keeping them alive as long as possible isn't really something client authors can rely on.

I'm actually more confused now than before. I always assumed that evaluate requests had to be re-sent on every stopped event and values were otherwise invalid.

Now it seems they might still be valid? Hard to know what to do with that info tbh.

@int19h
Copy link

int19h commented Feb 14, 2024

It seems to me that this particular use case really needs explicit lifetime management in form of some kind of "releaseVariableReference" request that client can issue to indicate that it's done with it. Otherwise, there's just no sensible way to use it, since the guarantee here is weak enough that in practice the variable may well be gone by the time "output" even is received and processed.

This would actually be good to have for other reasons. In the Python debugger, as an extension, we provide the ability to issue "evaluate" requests (without "frameId") even while debugger is running - this turned out to be useful for things that build on top of the debugger, such as Jupyter notebooks, as a generic facility to run some code inside the debuggee to retrieve values or for side effects. But, due to lifetime issues around variable references, we had to restrict this feature to returning stringified values only. Allowing manual lifetime control would allow for structured data, making it so much more flexible.

Also note that there is a distinction here between lifetime of the reference (i.e. variable ID), and lifetime of the actual variable itself. Depending on the runtime, lifetime of the variable might not be something that debugger can even control. In such cases, I think it's fine for "variables" request to return an error; the important thing is that variable ID is guaranteed to refer to the same variable throughout, and not be reused for another unrelated variable until the client explicitly releases it. This kind of race condition seems to be allowed by the present wording of the spec, or at least, I cannot find any verbiage that prohibits debug adapters from reusing ID once variable lifetime ends for whatever reason.

@connor4312
Copy link
Member Author

connor4312 commented Feb 15, 2024

I always assumed that evaluate requests had to be re-sent on every stopped event and values were otherwise invalid.

It's valid to send evaluate requests when not stopped, i.e. with an undefined frameId. Of course whether a particular DA and language can support this may vary.

It seems to me that this particular use case really needs explicit lifetime management in form of some kind of "releaseVariableReference" request that client can issue to indicate that it's done with it. Otherwise, there's just no sensible way to use it, since the guarantee here is weak enough that in practice the variable may well be gone by the time "output" even is received and processed.

That sounds like a good idea. However it might be difficult at this point for debug adapter authors to depend on, since they don't know if a consumer will send a "release" request/event. While we could hint it in InitializeRequestArguments, but I'm not sure it would be sufficient for your use case since other consumers can plug into the debug session and might not respect it.

Something that the Chrome Devtools protocol does is allowing the client to specify an objectGroup when calling functions/evaluating code to determine a lifetime in which the return value should be allocated, and it has a releaseObjectGroup method that can later be called. However I'm not sure how portable this is.

What do you think?

the important thing is that variable ID is guaranteed to refer to the same variable throughout, and not be reused for another unrelated variable until the client explicitly releases it.

Yes, that's a good distinction to make -- it also relates to "Keeping them alive as long as possible isn't really something client authors can rely on." This has always been the case, and if the user tries to interact with a variable and that request results in an error, the client should display an error. One easy case in VS Code is logging an object and ending the debug session. Attempting to then expand the object properties shows an error.

@int19h
Copy link

int19h commented Feb 15, 2024

The problem with client-initiated lifetime management protocol is that it inherently doesn't work with cases where the adapter generates a variable ID out of the blue (as is the case with "output" events with variables). On the other hand, requiring explicit lifetime management for existing parts of DAP would be a breaking change, so it's not really an option here, either. I don't see any way out of this conundrum short of adding a new, distinct property to "output" for the DA specifically for variables that require such lifetime management (and the corresponding capability etc, so that such messages are only sent to clients that are aware of the requirement). But for "evaluate" requests, having some kind of property that would tell the adapter that the resulting variable reference should never be auto-disposed seems like a sensible approach to me.

And yes, I think it would help a lot if all IDs were non-reusable. Is it viable, though, given that the spec limits IDs to 32-bit ints in practice? It feels like a typical debug session, with user manually triggering actions that cause IDs to be generated, would be extremely unlikely to exhaust 2**32 IDs for any kind of DAP entity. But the client needs not be controlled by a user; scripted DAP use is also a thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OutputEvent variablesReference lifetime clarification
6 participants