rpc: Fix deadlock produced during server shutdown #2966
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #2966 +/- ##
==========================================
- Coverage 84.86% 84.78% -0.08%
==========================================
Files 329 328 -1
Lines 43028 43047 +19
==========================================
- Hits 36515 36498 -17
- Misses 5028 5066 +38
+ Partials 1485 1483 -2
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
AnnaShaleva
reviewed
Apr 13, 2023
cthulhu-rider
force-pushed
the
bugfix/2896-rpc-shutdown-deadlock
branch
from
7267d65
to
9707efe
Compare
cthulhu-rider
force-pushed
the
bugfix/2896-rpc-shutdown-deadlock
branch
from
9707efe
to
b53b1bd
Compare
AnnaShaleva
reviewed
Apr 14, 2023
AnnaShaleva
reviewed
Apr 14, 2023
cthulhu-rider
force-pushed
the
bugfix/2896-rpc-shutdown-deadlock
branch
from
b53b1bd
to
4926d8b
Compare
AnnaShaleva
reviewed
Apr 15, 2023
There is an existing problem with RPC server shutdown freeze after start failure due to some init actions (at least HTTP listen) described in #2896. Add dedicated unit test which checks that `Shutdown` returns within 5s after `Start` method encounters internal problems. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Previously RPC server could never be shut down completely due to some start precondition failure (in particular, inability to serve HTTP on any configured endpoint). The problem was caused by next facts: * start method ran subscription routine after HTTP init succeeded only * stop method blocked waiting for the subscription routine to return Run `handleSubEvents` routine on fresh `Start` unconditionally. With this change, `Shutdown` method won't produce deadlock since `handleSubEvents` closes wait channel. Refs #2896. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Previously RPC server shutdown procedure listened to the execution channel and stopped at the first element that arrived in the queue. This could lead to the following problems: * stopper could steal the execution result from subscriber * stopper didn't wait for other subscription actions to complete Add dedicated channel to `Server` for subscription routine. Close the channel on `handleSubEvents` return and wait for signal in `Shutdown`. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
If `StartWhenSynchronized` is unset in config, `node` command runs RPC service instantly. Previously there was a ground for deadlock. Command started RPC server synchronously. According to server implementation, it sends all internal failures to the parameterized error channel. Deadlock occured because main routine didn't scan the channel. Run `rpcsrv.Server.Start` in a separate go-routine in `startServer`. This prevents potential deadlock caused by writing into unread channel. Fixes #2896. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider
force-pushed
the
bugfix/2896-rpc-shutdown-deadlock
branch
from
4926d8b
to
e29c33e
Compare
AnnaShaleva
approved these changes
Apr 17, 2023
roman-khimov
approved these changes
Apr 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
special attention to b6e34b2, maybe I missed the hidden meaning