Add a way to stop servers #386
Conversation
|
Related to #377 (not sure whether resolves or not) |
There was a problem hiding this comment.
LGTM,
For WebSockets the connection will be reset and that should be sufficient I think, then for subscriptions we will send a message when each subscription is closed but I guess we could test it with our WebSocket client to be sure how it's handled when the server is closed.
For HTTP then hyper will take care of that for us and I suppose all pending requests/futures then are "waited" to completed then it's terminated (no new requests are accepted)?
Yep, that's my understanding as well. |
http-server/src/tests.rs
Outdated
| .expect("Server stopped with an error"); | ||
|
|
||
| // After server was stopped, attempt to stop it again should result in an error. | ||
| assert!(stop_handle.stop().with_default_timeout().await.unwrap().is_err()); |
There was a problem hiding this comment.
Can we assert that we actually get the AlreadyStopped error here? I get that it is the only possible error, but if the code is ever changed in the future this may not be true anymore.
There was a problem hiding this comment.
sorry I didn't realize that StopHandle was not exported when I reviewed the PR initially because mod server is private thus those must be exported to be usable.
Follow-up for us to investigate is if it makes sense to cancel the background task for that already started connection too.
* master: (21 commits) New proc macro (#387) Streaming RpcParams parsing (#401) Set allowed Host header values (#399) Synchronization-less async connections in ws-server (#388) [ws server]: terminate already established connection(s) when the server is stopped (#396) feat: customizable JSON-RPC error codes via new enum variant on `CallErrror` (#394) [ci]: test each individual crate's manifest (#392) Add a way to stop servers (#386) [jsonrpsee types]: unify a couple of types + more tests (#389) Update roadmap link in readme (#390) Cross-origin protection (#375) Method aliases + RpcModule: Clone (#383) Use criterion's async bencher (#385) Async/subscription benches (#372) send text (#374) Fix link to ws server in README.md (#373) Concat -> simple push (#370) Add missing `rt` feature (#369) Release prep for v0.2 (#368) chore(scripts): publish script (#354) ...
I didn't dig into stopping all the active subscriptions, since I'm not sure whether it must be a part of the public interface (it's not a trivial matter, since the interface provides flexibility to implement subscription maintenance in any way you want, and doing something without proper notification to user may result in a bad DevEx).