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

Clean up/fix locking in handleReturn #395

Merged
merged 2 commits into from Dec 21, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Dec 20, 2022

This cleans up/fixes some of the locking towards the end of handleReturn; previously this was super convoluted, in part because we'd just put the whole rest of the function in a giant go func() {...}() at some point to solve a deadlock without cleaning it up in a more structural way; see the now-deleted TODO(cleanup).

This also adds a few comments related to the reasoning behind some conditionals, which are a bit obtuse. I think this probably could be cleaned up further, but this maintains the existing behavior while documenting it a bit better at least.

Finally, while doing the above I noticed that sendMessage() was being called without holding c.lk, so I added a call to syncutil.With to fix that.

After this patch there are only three remaining calls to syncutil.Without in the rpc package.

This is a little less convoluted, and avoids some uses of
syncutil.Without. No functional change.
This is a requirement per the contract, but we weren't doing it here.
rpc/rpc.go Show resolved Hide resolved
@zenhack
Copy link
Contributor Author

zenhack commented Dec 20, 2022

You might want to look at the individual commits instead of the whole diff, which is horrible. The latter commit indented a big block of code which git never seems to understand very well.

@zenhack
Copy link
Contributor Author

zenhack commented Dec 20, 2022

Source of the failure isn't immediately obvious to me; I'll stare at it in the morning (though I would hazard a guess it's been there and is not related to this patch).

@zenhack
Copy link
Contributor Author

zenhack commented Dec 21, 2022

Last failure looks like #377, so not the same bug. Once we've nailed down these more structural improvements, we should probably do another pass of "get CI to stop failing"

@zenhack zenhack merged commit 73ff1cb into capnproto:main Dec 21, 2022
@zenhack zenhack deleted the handleReturn-locking branch December 21, 2022 02:15
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.

None yet

2 participants