-
-
Notifications
You must be signed in to change notification settings - Fork 524
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
Only send pong if OPEN. #1429
Only send pong if OPEN. #1429
Conversation
6fb32fa
to
e264d6a
Compare
Thank you for reporting this bug! The legacy implementation includes this logic, suggesting that this is the right fix in the Sans-I/O layer. I'd like to double check and add a test before merging. If you're able to reproduce easily, would you be able to provide debug logs by any chance? That would help me understand exactly the scenario in which this happens. In short, you can enable debug logs with: import logging
logging.basicConfig(format="%(asctime)s %(message)s", level=logging.DEBUG, |
So I've realised that this is just an exception being logged by websockets within the context of recv() - the caller still receives the expected message. I can replicate this reliably in the REPL, relevant logs as follows:
The logic sequence is as follows:
EDIT: formatting |
I discovered that I wasn't closing the websocket server-side after sending the 'stopped' message - I was just dereferencing it. Fixing that seemed to fix the client behaviour. That said, I still think the client shouldn't attempt to send a pong message if it is not in the OPEN state. |
Thank you for the details - that's very useful! |
The commit message for b2a95c4 suggests that the current behavior of the Sans-I/O layer is intentional. Mmm. |
For clarity, the problem happens in the following scenario:
This can happen either on the client side or the server side. |
Above I described the scenario where the connection state is CLOSING because we initiated the closing handshake. The connection could also be closing because the other side initiated the closing handshake. However, in that case, websockets will never read a Ping frame after a Close frame — because it stops reading entirely after a Close frame: websockets/src/websockets/protocol.py Lines 677 to 681 in 5209b2a
|
RFC 6455 says:
Note that it doesn't say "... unless it already sent a Close frame". Also, it says:
This implies that it's OK to send more control frames, such as Pong frames. (Since Ping/Pong are primarily a keepalive / heartbeat mechanism, it doesn't seem very useful to send a Pong after a Close. In fact, on the other side of the connection, websockets will ignore the Pong in this scenario — as mentioned in the previous comment it stops reading data frames and control frames after a Close frame. But, hey, it's legal.) Also, there's comments in the current test suite pointing out that the current behavior doesn't match the spec. All in all, I'm leaning towards following the spec more closely, meaning allowing sending ping and pong after close. |
Superseded by #1435. |
In an earlier comment, you said:
If you could double-check that #1435 fixes the problem for you, that would be most useful :-) |
Sorry for delay - I just confirmed that your fix works for my test case. Thanks! Any feel for when it might make it into a release? |
If the server sends a message and then closes the connection, it's possible for the client to fail to
recv()
the message due to an attempt to send a pong message during the CLOSING state. The following is printed:I found that modifying
websockets/protocol.py
to only send a pong message during the OPEN state allowed me to .recv() the message in this case.