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

WIP: avoid deadlocks wrt. embargo #458

Merged
merged 7 commits into from
Feb 24, 2023
Merged

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Feb 20, 2023

This patch:

  • Moves the answerQueue (and structReturner) types from the server package into the main package, and exports them.
  • Reworks the implementation of embargo in the rpc package to use answerQueue; this makes the queue unbounded instead of having Send & Recv just block, which should head off some potential deadlocks which right now I think require multiple connections to trigger.

Marking this WIP because there are a few bits that are still a little dodgy, both stylistically and in terms of correctness. But the tests pass.

@zenhack zenhack marked this pull request as draft February 20, 2023 03:36
This should head off some potential deadlocks. Still WIP though; see the
FIXME comment in particular.
lthibault
lthibault previously approved these changes Feb 20, 2023
Stared at this a bit; I don't think anything needs to be done, since
Shutdown() shouldn't get called if there are outstanding calls.
@zenhack zenhack marked this pull request as ready for review February 24, 2023 05:30
The body of this was more verbose than it needed to be, and upon cutting
it down I realized it could just be inlined, so here we go.
@zenhack
Copy link
Contributor Author

zenhack commented Feb 24, 2023

Ok, this is ready now.

@zenhack
Copy link
Contributor Author

zenhack commented Feb 24, 2023

I have no idea what to make of this: https://github.com/capnproto/go-capnproto2/actions/runs/4259670598/jobs/7412098591#step:5:636

It looks like a compile error? I'm going to re-run, but that's bizzare either way. It definitely builds locally.

@lthibault lthibault merged commit 0170721 into capnproto:main Feb 24, 2023
@zenhack zenhack deleted the answerqueue branch February 24, 2023 21:30
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.

2 participants