-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm][debugger] Fixing async locals in nested ContinueWith blocks #56911
Conversation
Tagging subscribers to this area: @thaystg Issue DetailsAdding test to close #41984
|
Tagging subscribers to 'arch-wasm': @lewing Issue Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just some feedback
src/mono/wasm/debugger/tests/debugger-test/debugger-async-test.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Ankit Jain <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
var match = regex.Match(fieldName); | ||
if (match.Success) | ||
asyncLocal["name"] = match.Groups[1].Value; | ||
asyncLocalsFull.Add(asyncLocal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this still be valid without a name? If not, then we shouldn't add it to asyncLocalsFull
, and just skip it.
@@ -1729,14 +1786,7 @@ public async Task<JArray> StackFrameGetValues(SessionId sessionId, MethodInfo me | |||
retDebuggerCmdReader.ReadByte(); //ignore type | |||
var objectId = retDebuggerCmdReader.ReadInt32(); | |||
var asyncLocals = await GetObjectValues(sessionId, objectId, true, false, false, false, token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what does this return? reference to the generated class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you! 👍
The changes did not impact the lanes that had failed. |
ContinueWith
block #41984