Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test: WebSocket close behavior. #3093

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Aug 1, 2023

PR Type

Test/Question/Other

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

This PR emphasises my question regarding proper behavior of WebSocket close procedure.
It seems like neither receiving close and sending it back to client nor initiating close and receiving it back from client (close handshake) does not cause web socket stream to finish and TCP connection released.

I'm not sure if it's my expectation from API is wrong or there is a bug. But anyway the question is what is the proper way to terminate WebSocket (except ctx.stop() which seems to terminate connection ungracefully, possibly loosing pending messages)?

I've inspected code and it seems like there are some parts which are intended to do some special handling of web socket closure, but this code seems to be never executed. E.g. there is Message::Close with single usage across actix-web repo, so it's not created anywhere:

Poll::Ready(Some(Ok(Message::Close))) => {
*this.state = State::FlushAndStop;
return true;
}

There is also some code within WebsocketContextFut to mark WebSocket as closed, which marks WS as closed when None is read from messages queue:

while let Some(item) = this.fut.ctx().messages.pop_front() {
if let Some(msg) = item {
this.encoder.encode(msg, &mut this.buf)?;
} else {
this.closed = true;
break;
}
}

while there is no possibly to write None to outgoing messages queue:
#[inline]
pub fn write_raw(&mut self, msg: Message) {
self.messages.push_back(Some(msg));
}
/// Send text frame
#[inline]
pub fn text(&mut self, text: impl Into<ByteString>) {
self.write_raw(Message::Text(text.into()));
}
/// Send binary frame
#[inline]
pub fn binary(&mut self, data: impl Into<Bytes>) {
self.write_raw(Message::Binary(data.into()));
}
/// Send ping frame
#[inline]
pub fn ping(&mut self, message: &[u8]) {
self.write_raw(Message::Ping(Bytes::copy_from_slice(message)));
}
/// Send pong frame
#[inline]
pub fn pong(&mut self, message: &[u8]) {
self.write_raw(Message::Pong(Bytes::copy_from_slice(message)));
}
/// Send close frame
#[inline]
pub fn close(&mut self, reason: Option<CloseReason>) {
self.write_raw(Message::Close(reason));
}

The only part of close which seems to work as expected is when underlying connection is closed by client app and handled here:

match Pin::new(&mut this.stream).poll_next(cx) {
Poll::Ready(Some(Ok(chunk))) => {
this.buf.extend_from_slice(&chunk[..]);
}
Poll::Ready(None) => {
*this.closed = true;
break;
}
Poll::Pending => break,
Poll::Ready(Some(Err(e))) => {
return Poll::Ready(Some(Err(ProtocolError::Io(io::Error::new(
io::ErrorKind::Other,
format!("{}", e),
)))));
}
}

So maybe maintainers could kindly provide answers to questions I've raised here:

  1. Is provided modifications to tests should be passed or not? (currently not passed)
  2. If no, what is the proper way to gracefully close WebSocket connection by server side?

@robjtede robjtede added B-semver-norelease change that does not require a release A-actors project: actix-web-actors labels Jun 11, 2024
@centromere
Copy link

Will this get investigated? If I run f = stream.next().await and the remote peer sends a TCP FIN, f will not resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-actors project: actix-web-actors B-semver-norelease change that does not require a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants