PowerShellEditorServices
PowerShellEditorServices copied to clipboard
PowerShell
Invalidate variables cache in SetVariableAsync and in EvaluateExpressionAsync to have actual variables data
PR Summary
If the user modifies, adds, or removes a variable (e.g., with SetVariable or Evaluate request) and then sends Variables request (which calls GetVariables), they get unmodified variables set from the cached variables field.
I have added FetchStackFramesAndVariablesAsync calls in SetVariableAsync and in EvaluateExpressionAsync to invalidate the cache and get actual data on Variables request
This is probably related to #58, but I am not sure.
PR Context
We are working on adding debugger support to https://github.com/ant-druha/intellij-powershell
@Fantoom do you think the CI failure is related to https://github.com/PowerShell/PowerShellEditorServices/issues/2170?
Looks like the same issue. I have a hypothesis (not yet checked with the sources): what if a test executes some operation that would cause that exception, but it is currently masked by the fact that PSES doesn't re-evaluate the variables after each step?
In that case, it is possible that there's a real error that is hidden from the test.
@andyleejordan
@Fantoom do you think the CI failure is related to #2170?
I have debugged it and have found that after SetVariable, FetchStackFramesAndVariablesAsync fetches variables with two more variables called "$__psEditorServices_CallStack." And that changes the referenceIds of the scopes.
I see two ways of solving this:
- Users should clear their own cache after SetValue/Evaluate and refetch with GetVariables. I'm not sure if this violates the DAP or not
- Update variable value in the cache instead of refetching it. I am not sure if this reflects all the changes or not
After a bit of additional digging (disclaimer: I am a supervisor of @Fantoom), we found these lines in the DAP spec:
Some complex structural objects such as scopes or variables are not returned directly with their containers (stack frames, scopes, variables), but must be retrieved with separate scopes and variables requests based on object references. An object reference is an integer in the open interval (0, 231) assigned to objects by the debug adapter.
To simplify the management of object references in debug adapters, their lifetime is limited to the current suspended state. Once execution resumes, object references become invalid and DAP clients must not use them. When execution is paused again, object references no longer refer to the same objects. This means that a debug adapter can easily use sequentially assigned integers for different objects and reset the counter to 1 when execution resumes.
Variable references not related to the current suspended state, such as those from evaluate requests or in output events, should be preserved as long as feasible for the debug adapter so that the client may later inspect them.
Please note that other object references like threadId do not have limited lifetime because that would prevent something like the pause request from working in the running state.
I am not very sure, but to me it looks like, unfortunately, this PR breaks the spec expectation that the variable ids are preserved between the suspend points (and effectively introduces a new suspend state after any variable evaluation).
I'd rely on your judgement, folks, on what to do with that further. We might have to redesign this part, to allow some kind of partial update of the suspend state, preserving the variable ids?
@ForNeVeR I have this on my radar, just getting back to work after breaking my hand! Will reply more in depth soon.
After a bit of additional digging (disclaimer: I am a supervisor of @Fantoom), we found these lines in the DAP spec:
Some complex structural objects such as scopes or variables are not returned directly with their containers (stack frames, scopes, variables), but must be retrieved with separate scopes and variables requests based on object references. An object reference is an integer in the open interval (0, 231) assigned to objects by the debug adapter. To simplify the management of object references in debug adapters, their lifetime is limited to the current suspended state. Once execution resumes, object references become invalid and DAP clients must not use them. When execution is paused again, object references no longer refer to the same objects. This means that a debug adapter can easily use sequentially assigned integers for different objects and reset the counter to 1 when execution resumes. Variable references not related to the current suspended state, such as those from evaluate requests or in output events, should be preserved as long as feasible for the debug adapter so that the client may later inspect them. Please note that other object references like threadId do not have limited lifetime because that would prevent something like the pause request from working in the running state.
I am not very sure, but to me it looks like, unfortunately, this PR breaks the spec expectation that the variable ids are preserved between the suspend points (and effectively introduces a new suspend state after any variable evaluation).
I'd rely on your judgement, folks, on what to do with that further. We might have to redesign this part, to allow some kind of partial update of the suspend state, preserving the variable ids?
@JustinGrote don't forget this part!
Sorry, I meant to say the test should be fine, not that the pr is ready to be merged
@ForNeVeR I have this on my radar, just getting back to work after breaking my hand! Will reply more in depth soon.
Funny story: I just broke my other arm. 🫠
This is not at all funny, and please get well soon!
For the record, I still believe that IntelliJ and probably VSCode integrations could benefit from this change, but my concerns about the correctness still stand. Absolutely open for discussion of other possibilities, or these changes as well.
I'd rely on your judgement, folks, on what to do with that further. We might have to redesign this part, to allow some kind of partial update of the suspend state, preserving the variable ids? @JustinGrote don't forget this part!
How I wanted to redo this to match the spec is to convert our variable/etc. debug state classes to records and make them immutable, generate them on pause and dispose them on resume. Then we can decide if some things can be reasonably cached for performance, but the generation/disposal will happen every time so that there are fresh IDs (I doubt it'll performance issue to do so)
convert our variable/etc. debug state classes to records and make them immutable, generate them on pause and dispose them on resume.
The key moment here is whether you consider a user-triggered evaluation a pause+resume or not.
If you do — means that all the current variables will be removed and replaced with new on any evaluation (which is not ideal). If you don't — means that sometimes we can update a state of existing variable (or emit a variable with same id but new value), e.g. if a user chose to evaluate an expression $val = 123 or $val.internalState = $newInternalState.
https://microsoft.github.io/debug-adapter-protocol/specification#Requests_SetVariable
A setvariable action isn't a net-new ID, it's updating an existing ID, so flushing the vars isn't necessary so I agree we need to make sure the cache is updated for that particular ID.
That does leave a hole for if someone does something like $var = ($var2 = 5), we would miss updating $var2. We could AST the expression for all vars but then that doesn't work for computed var names like setting the name in Set-Variable, so it's probably worth the expense to invalidate the entire cache since it's a relatively uncommon activity. However now we have the problem that new requests would have new IDs and so if you went to go do another set-variable, now that other var doesn't exist anymore.
Ideally the var records lifetime is tied to their presence in a cache collection and invalidating them should dispose them, and this would just be triggering on that workflow cycle.