Fix close edge cases #1908
Fix close edge cases #1908
Conversation
Ensure that `socket.end()` is called if an error occurs simultaneously on both peers. Refs: #1902
lpinca
force-pushed
the
fix/close-edge-cases
branch
from
bdfb346 to
614aa04
Compare
| if (this._closeFrameSent && this._closeFrameReceived) this._socket.end(); | ||
| if ( | ||
| this._closeFrameSent && | ||
| (this._closeFrameReceived || this._receiver._writableState.errorEmitted) |
There was a problem hiding this comment.
@pimterry I think that adding this._receiver._writableState.finished here and below along with the -2 ready state would also address #1902 (comment).
Anyway I would prefer to keep it as is because:
- A socket
'end'event with no close frame means that the remote peer is doing something wrong. - We are consistent with browsers behavior. The connection is closed immediately with no close frame sent back.
There was a problem hiding this comment.
Yes, I'm convinced! 👍 Given all the extra complexity involved and that the spec doesn't mind either way, in the case where the remote peer half-closes their end unexpectedly I think it's totally reasonable to drop all our outgoing data, fully close the socket, and treat it as an abnormal websocket close.
| if (this._closeFrameSent && this._closeFrameReceived) this._socket.end(); | ||
| if ( | ||
| this._closeFrameSent && | ||
| (this._closeFrameReceived || this._receiver._writableState.errorEmitted) |
There was a problem hiding this comment.
Yes, I'm convinced! 👍 Given all the extra complexity involved and that the spec doesn't mind either way, in the case where the remote peer half-closes their end unexpectedly I think it's totally reasonable to drop all our outgoing data, fully close the socket, and treat it as an abnormal websocket close.
|
|
||
| if ( | ||
| this._closeFrameReceived || | ||
| this._receiver._writableState.errorEmitted |
There was a problem hiding this comment.
Nitpicking: since both of these checks are effectively the same (sent is just omitted here because we know it's true) it might be clearer if this gets extracted into some _connectionCompleted() internal method, so that the logic here is a bit clearer and easier to check in future.
Doesn't affect the functionality either way of course, so up to you, it'd just make it a bit easier to tell what's going on.
There was a problem hiding this comment.
Yes, it makes sense. I'll keep it as is now as I can't think of a good name for the function and eventually change it later.
|
Thank you for the review. |
Ensure that
socket.end()is called if an error occurs simultaneously onboth peers.
Refs: #1902