fix(ws server): fix shutdown on connection closed #1103
Conversation
server/src/transport/ws.rs
Outdated
| }); | ||
|
|
||
| let pending = pending_calls.for_each(|_| async {}); | ||
| let disconnect = disconnect_stream.for_each(|_| async {}); |
There was a problem hiding this comment.
it's possible that the connection is terminated during the graceful shutdown and this ensures that it is aborted once the close message is sent.
however, this is slightly error prone if the no proper close is sent but yeah would neat if soketto had something to indicate when the connection is closed
| @@ -272,11 +272,11 @@ pub(crate) async fn background_task<L: Logger>( | |||
| stopped = stop; | |||
| permit | |||
| } | |||
| None => break Ok(()), | |||
| None => break Ok(Shutdown::ConnectionClosed), | |||
There was a problem hiding this comment.
Am I right in thinking that:
Stoppedmeans the user has manually stopped the server, so we want to gracefully close the eonnctionConnectionClosedmeans the connection was closed for some other reason (eg network issue or whatever)
There was a problem hiding this comment.
yes, this is only fails if the send_task has been completed and the receiver of the bounded channel has been dropped.
this can only occur if the send_ping fails I think i.e, connection closed
server/src/transport/ws.rs
Outdated
| tokio::select! { | ||
| _ = pending => (), | ||
| _ = disconnect => (), | ||
| } |
There was a problem hiding this comment.
So basically, whatever finishes first out of disconnect (which looks like any final things to be received?) and pending (whihc looks like anything to be sent back?) will lead to the thing ending?
There was a problem hiding this comment.
I find it a bit hard to follow the logic here!
There was a problem hiding this comment.
yeah, it's way to make it possible to detect whether connection has terminated while we try to wait for the pending futures to complete
…' into na-fix-flaky-ws-conn-closed-test
…' into na-fix-flaky-ws-conn-closed-test
server/src/tests/ws.rs
Outdated
| client.send(req).with_default_timeout().await.unwrap().unwrap(); | ||
| } | ||
| // Assumption: the calls would be ACK:ed by server after 10 seconds (no way knowing that except parsing CLI output) | ||
| tokio::time::sleep(std::time::Duration::from_secs(10)).await; |
There was a problem hiding this comment.
dq: I remember from the substrate CI that these constants could be exceeded on overcommited days. Should we increase the timeout a bit here, or this should be sufficient?
There was a problem hiding this comment.
I don't know what I was thinking, I could embed mpsc::Sender in the method callback and await for 10 messages
Nice to get rid of sleeps, those shouldn't be used if possible :D
|
@jsdw can you have another look if it's easier to read now? Did some minor refactoring |
* chore: release v0.18.1 * fix(ws server): fix flaky shutdown test * adjust changelog for #1103 * Update CHANGELOG.md
No description provided.