Remove redundant sendVarDelete call that throws an error

[duyhihi91]

this is the updated description and summary, as this change was progressed the design and implementation changed substantially. See collapsed section below for original description.

This sendVarDelete is rarely reachable, generally only reachable when the same variable name is used in two different scopes within the same method. See use of x variable in
src/integration-tests/test-programs/watch_local_scope_transition.c for an example.

When debugging such code, and the x was in the watch window, the now removed sendVarDelete would run, but the var was already deleted by the varManager.removeVar on the line above. So the sendVarDelete would generate an exception, causing the x not to be evaluated.

Included in this change is a test that demonstrates the use case, the new test evaluate request - watch local variable across lexical scope transition in evaluate.spec.ts.

Original message

Summary: Improvement/wrap send var delete in try/catch after remove var in do evaluate request

For ECA reasons, this is a continuation of #509

Background: removeVar(...) called sendVarDelete(...) inside itself.
In case of doEvaluateRequest(...) method, it will throw "object not found" error when calling sendVarDelete(...) after removeVar(...).

Solution: add try/catch to safely wrap the sendVarDelete(...) to swallow the "object not found" error.
Other errors still be catch in catch (err) block in the end of doEvaluateRequest(...) as currently.

Reason of NOT removing sendVarDelete(...) in doEvaluateRequest(...):
1/ In future, if a person removes sendVarDelete(...) inside removeVar(...) -> sendVarDelete(...) still need to be called after removeVar(...) - same as current development.
2/ Why I did not remove sendVarDelete(...) inside removeVar(...) : because removeVar(...) is used at other place , also maybe used in third party code. If I change removeVar(...) it will impact their implementation.

[duyhihi91]

Hi @jonahgraham ,

I created the Pull Request to solve the error of "object not found" in calling sendVarDelete after using removeVar(...) method, it causes duplicated deleting.
The solution is wrapping to swallow the "object not found" error to continue the other processing alternatively, other error (if any) still be thrown as previously.

Could you please help me to review it?

(I add the test script also in doEvaluateRequest.spec.ts file)

Thank you so much.

[jonahgraham]

Can you provide a way to reproduce this error? I still don't understand how users see this problem. It also seems that none of the tests cover these lines of code.

I have provided some line comments, but the key thing I don't understand is whether the sendVarDelete actually can succeed. And if it can never succeed, then why can't we just delete it.

[duyhihi91]

Can you provide a way to reproduce this error? I still don't understand how users see this problem. It also seems that none of the tests cover these lines of code.

I have provided some line comments, but the key thing I don't understand is whether the sendVarDelete actually can succeed. And if it can never succeed, then why can't we just delete it.

Thank you @jonahgraham for reviewing and pointing out issues.
I make a reproduce to describe the issue (it happends to LOCAL variables in Watch View only).
I checked and updated the solution from wrapping sendVarDelete(...) in try/catch to removing the duplicated deleting.
Also, I revised the test of "doEvaluateRequest.spec.ts" & "evaluate.spec.ts", added the new test "varManager.removeVar.spec.ts" to notify person in future who have intend to remove sendVarDelete(...) in removeVar(...).

[jonahgraham]

I was pinged offline about this and realized that I had not submitted my below review. To avoid further delays I will apply the removal of console.log myself and this can be merged.

[jonahgraham]

Updated title and original description to be:

This sendVarDelete is rarely reachable, generally only reachable when the same variable name is used in two different scopes within the same method. See use of x variable in
src/integration-tests/test-programs/watch_local_scope_transition.c for an example.

When debugging such code, and the x was in the watch window, the now removed sendVarDelete would run, but the var was already deleted by the varManager.removeVar on the line above. So the sendVarDelete would generate an exception, causing the x not to be evaluated.

Included in this change is a test that demonstrates the use case, the new test evaluate request - watch local variable across lexical scope transition in evaluate.spec.ts.