Fix 271#288
Merged
Merged
Conversation
...after merging in main, this no longer built, due to the change removing struct wrappers.
The callback is run without holding c.mu, so we need to acquire it to mutate the questions table. I have not (to my knowledge) observed symptoms of this, but spotted it while debugging something else.
Silly mistake: when a pipelined question failed to send, we were rejecting the promise for the *original* question, not the new one we just failed to send -- so when the return message for the original came back, we got a panic. Solution is to reject q2, not q. Note that the tests also needed tweaking, because we don't expect no error, we just don't want a panic -- the error should reflect the canceled context.
Contributor
Author
|
While debugging this, I added some code to record a stack trace for the first call to Fulfill/Reject, which was helpful. I didn't keep it in because it means keeping a buffer around for each fulfilled promise, and the way I did it I just oversized it so it would be 1MiB per promise, but worth bearing in mind/maybe thinking about how we can make the system easier to introspect for stuff like this. |
lthibault
approved these changes
Aug 8, 2022
lthibault
left a comment
lthibault
left a comment
Collaborator
There was a problem hiding this comment.
That was quick! 😃
I'll run the tests on my machine after merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The actual fix is the last commit; see the commit message.
There are also a couple other improvements in this pr that I made along the way:
Note, this fixed both tests; I can't reproduce #272. @lthibault, are you still able to reproduce it on this branch?