Add a way to close the http server while waiting #437
Conversation
|
The CI failure is an odd one (and not related to this PR afaict). I could reproduce it once locally using rust stable but never with nighlty. After a |
Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
…dle them up in a tuple
* master: Add client transport features (#439)
|
@tomusdrw good to go? |
| executor.wait(); | ||
| if let Some(receivers) = self.done.take() { | ||
| for receiver in receivers { | ||
| let _ = receiver.wait(); |
There was a problem hiding this comment.
@dvdplm Is this wait sufficient to guarantee that all the threads owned by the server have cleaned up by the time wait returns? From what I understand it guarantees that the serving threads have been cleaned up here: https://github.com/paritytech/jsonrpc/blob/master/http/src/lib.rs#L565, but the tokio threads owned by the RpcEventLoop aren't guaranteed to have exited until this other wait returns from within the Executor->RpcEventLoop here: https://github.com/paritytech/jsonrpc/blob/master/server-utils/src/reactor.rs#L75
There was a problem hiding this comment.
I don't think we can wait for the Executor to finish - it could have been spawned externally or shared for some other tasks. I guess the issue might be that wait() only actually waits for the moment where we stop accepting new requests, not when we stop processing them.
Do you have a specific issue that this behaviour causes for you?
There was a problem hiding this comment.
@tomusdrw, @dvdplm thanks for the reply! I am running into issues writing test code for testing start/ restart behavior on the server. I have specific cleanup behavior that runs on things like filesystem objects in the Drop implementation of certain structs that are owned by Executor tasks. B/c shutting down the server doesn't guarantee the spawned Executor tasks have finished, then on restart of the server, there's potential for data races based on when these Executor tasks finish and these objects get dropped.
There was a problem hiding this comment.
Please do, but I think it's a tricky one. We need to distinguish between externally provided executor (where we don't really want to wait until it ends) and a one that we've spawned ourselves.
Ref. #331
This PR implements the idea put forward by @tomusdrw to add an additional
oneshotchannel to let theServerknow when the future is shut down, which lets us write an implementation ofServer::wait()that can be interrupted by an external signal.(Also includes a few fixed compiler warnings)