Fix race condition on wait()
#504
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
|
It looks like @foriequal0 signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
tomusdrw
reviewed
Oct 16, 2019
http/src/lib.rs
Outdated
| @@ -565,7 +565,10 @@ fn serve<M: jsonrpc::Metadata, S: jsonrpc::Middleware<M>>( | |||
| })) | |||
| .map_err(|_| ()) | |||
| }) | |||
| .and_then(|_| done_tx.send(())) | |||
| .and_then(|(_, other)| { | |||
| drop(other); | |||
There was a problem hiding this comment.
Could you add a comment what are we dropping here and why? Something like:
"We drop the Server first to prevent a situation where main thread terminates before the Server is properly dropped (see #504 for more details)".
There was a problem hiding this comment.
maybe s/other/server/ then?
ipc/src/server.rs
Outdated
| @@ -240,7 +240,8 @@ impl<M: Metadata, S: Middleware<M>> ServerBuilder<M, S> { | |||
| .buffer_unordered(1024) | |||
| .for_each(|_| Ok(())) | |||
| .select(stop) | |||
| .map(|_| { | |||
| .map(|(_, other)| { | |||
There was a problem hiding this comment.
Same here if you don't mind.
|
Thank you! I added comments to them. |
tomusdrw
approved these changes
Oct 16, 2019
dvdplm
approved these changes
Oct 16, 2019
http/src/lib.rs
Outdated
| @@ -565,7 +565,10 @@ fn serve<M: jsonrpc::Metadata, S: jsonrpc::Middleware<M>>( | |||
| })) | |||
| .map_err(|_| ()) | |||
| }) | |||
| .and_then(|_| done_tx.send(())) | |||
| .and_then(|(_, other)| { | |||
| drop(other); | |||
There was a problem hiding this comment.
maybe s/other/server/ then?
|
@foriequal0 please rebase as well while you address @dvdplm's grumble. |
Manually dropping the future passed from `Future::select` before sending `done_tx` prevents the race condition. `Future::select` pass the unresolved future, which is a `Server` holding `rpc_handler`, to the following callback. Therefore, it is dropped after the `done_tx.send(())` after the callback exits. It is possible that the thread that has executed `wait()`, which is usually the main thread, terminates before the thread sending `done_tx` drops `rpc_handler`. Static variables are destructed at the end of termination of the main thread, and then a segfault occurs when the thread dropping the rpc_handler accesses the variables.
foriequal0
added a commit
to foriequal0/codechain
that referenced
this pull request
Oct 18, 2019
The fix (paritytech/jsonrpc#504) has been merged to `master`. It'll take times to relase, so use the merged master directly.
foriequal0
added a commit
to foriequal0/codechain
that referenced
this pull request
Oct 18, 2019
The fix (paritytech/jsonrpc#504) has been merged to `master`. It'll take times to relase, so use the merged master directly.
foriequal0
added a commit
to CodeChain-io/codechain
that referenced
this pull request
Oct 18, 2019
The fix (paritytech/jsonrpc#504) has been merged to `master`. It'll take times to relase, so use the merged master directly.
Merged
foriequal0
added a commit
to foriequal0/codechain
that referenced
this pull request
Nov 8, 2019
The fix (paritytech/jsonrpc#504) has been merged to `master`. It'll take times to relase, so use the merged master directly.
foriequal0
added a commit
to CodeChain-io/codechain
that referenced
this pull request
Nov 11, 2019
The fix (paritytech/jsonrpc#504) has been merged to `master`. It'll take times to relase, so use the merged master directly.
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.
Manually dropping the future passed from
Future::selectbefore sendingdone_txprevents the race condition.Future::selectpass the unresolved future, which is aServerholdingrpc_handler, to the following callback. Therefore, it is dropped afterthe
done_tx.send(())after the callback exits.It is possible that the thread that has executed
wait(), which isusually the main thread, terminates before the thread sending
done_txdrops
rpc_handler.Static variables are destructed at the end of termination of the main
thread, and then a segfault occurs when the thread dropping the
rpc_handler accesses the variables.
I closed the previous PR (#501).
Sorry for the confusion.