server: graceful shutdown check Incoming::Closed
#1216
Conversation
niklasad1
force-pushed
the
na-graceful-shutdown-check-incoming-closed
branch
from
8743ed1
to
53b882b
Compare
| match receiver.receive(&mut data).await { | ||
| Ok(Incoming::Closed(_)) | Err(SokettoError::Closed) => None, | ||
| Ok(Incoming::Data(_) | Incoming::Pong(_)) => Some(((), (receiver, data))), | ||
| Err(e) => { |
There was a problem hiding this comment.
Just to check; do you no longer need to return None when Err(SokettoError::Closed) is a thing now?
There was a problem hiding this comment.
All errors except Closed are ignored here because we can't really know whether when it's closed or not otherwise.
We will keep the stream alive until either SokettoError::Closed or Incoming::Closed is returned back because some implementations may not do the proper WebSocket close handshake message such as the soketto client #871. Thus, we need to check both
However, soketto will eventually return SokettError::Closed after this https://github.com/paritytech/soketto/blob/master/src/connection.rs#L386-L388 so this extra check in this PR shouldn't matter in "practice" but we need to call receive one more time instead then also I like us to be explicit here which makes it easier to understand.
FWIW, it may also be a bunch of I/O errors that could terminate the connection as well but those are super-tricky to detect so we are relying on the close stuff... and that soketto regards EOF errors as Closed
There was a problem hiding this comment.
Oohh I actually missed that you were still paying attention to SokettoError::Closed anyway, so yup all makes sense to me now; thanks for the explanation! :)
* server: graceful shutdown check `Incoming::Closed` * Update server/src/transport/ws.rs
No description provided.