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

Socket instance doesn't wait for all the packets in writeBuffer to be sent before closing the websocket transport. #648

Closed
h-fotakiev opened this issue Jun 7, 2022 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@h-fotakiev
Copy link

h-fotakiev commented Jun 7, 2022

Describe the bug
When socket.close() is called, if there are pending packets in the writeBuffer, the transport close is postponed until a "drain" event for the socket is emitted. On socket.flush() the pending packages in the writeBuffer are sent via transport.send(wbuf), and immediately after this line, the "drain" event is emitted synchronously and closes the transport. The problem is that in the case of websocket transport, the transport.send function is asynchronous(it sends each packet and waits for confirmation that the packet was sent successfully, before sending the 2nd packet, and so on), therefore if the buffer consists of 4 packets, 1 will be delivered to the client, but after that, the transport will be closed and the rest of the packets won't be sent.

To Reproduce

  1. Make multiple(more than 2) synchronous calls of socket.write() with some data.
  2. Call socket.close() synchronously after the previous write calls.

Engine.IO server version: 6.X.X

Server

socket.write("2[\"force_disconnect\",\"multiple_clients_connected\"]"); // initiates a buffer of 1 packet; sent successfully and received by client
socket.write("1,"); // initiates a buffer of 3 packets; sent successfully and received by client
socket.write("1/foo,"); // part of the 3 packets buffer; not sent due to transport close
socket.write("1/bar,"); // part of the 3 packets buffer; not sent due to transport close
socket.close();

Expected behavior
The socket waits for all of the messages in the buffer to be sent before closing the transport.

Additional context
This bug is breaking the Socket.IO v4 library. Users can connect to multiple "namespaces". A user's connection can be closed by the server by calling sioSocket.disconnect(true). This essentially sends a disconnect packet to all the "namespaces" a user has been connected to, so that the client knows, that it's being disconnected before the transport is closed, and it's an intentional disconnect by the server. In that case, the client shouldn't attempt to reconnect automatically because it's not an unexpected transport close. The example provided above is the exact order of calls that the Socket.IO library executes when disconnecting a client who was connected to namespaces "foo" and "bar". From these 4 packets in the example, after sending the first 2 messages, the transport is being closed by the socket instance, which prevents the 2 remaining messages from being sent, preventing the sioSocket.disconnect(true) function from working properly, and ultimately causing a lot of clients-unintentionally-reconnecting-over-and-over-again problems with the Socket.IO library.

@h-fotakiev h-fotakiev added the bug Something isn't working label Jun 7, 2022
@darrachequesne
Copy link
Member

Thanks, I was able to reproduce the issue, it seems to be linked to ad5306a.

darrachequesne added a commit that referenced this issue Jan 10, 2023
…nection

This reverts commit [1], which was included in `engine.io@5.1.0` and
`socket.io@4.1.0`.

The WebSocket connection was closed before all packets were written
out, so for example when calling `socket.disconnect(true)` on the
Socket.IO server (which disconnect from all namespaces and close the
connection), the client would receive only the first disconnect packet
and kept trying to reconnect to the other namespaces.

The only difference with the previous implementation (pre 5.1.0) is
that the "drain" event gets only called once at the end, and not after
each packet.

[1]: ad5306a

Related: #648
@darrachequesne
Copy link
Member

This should be fixed by a65a047, included in version 6.3.0.

Please reopen if needed.

@darrachequesne darrachequesne added this to the 6.3.0 milestone Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants