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

Prevent reusing the socket after receiving a server_disconnect #647

Merged
merged 6 commits into from Nov 22, 2019
Merged

Prevent reusing the socket after receiving a server_disconnect #647

merged 6 commits into from Nov 22, 2019

Conversation

GaryWilber
Copy link
Contributor

@GaryWilber GaryWilber commented Nov 22, 2019

The client gets a "server_disconnect" message from the server for "IdleTime" and emits a "disconnect" for the socket. That "disconnect" doesn't close the socket until 2 seconds later. The client then decides to reconnect, and it ends up using that same socket (this a problem). It sends a "connect_document" message, but then the socket is closed by the server and the client waits forever.

When the client gets a "server_disconnect", mark the socket reference as closing. When deltaConnection.disconnect is called, the reference will be removed.

@vladsud
Copy link
Contributor

vladsud commented Nov 22, 2019

        socketReference.references++;

I believe this code path has to check for socket connection. The problem with relying on server_disconnect is that socket can be closed for any reason, including loss of connectivity.
I'd prefer a fix that take all reasons into account, and works on websocket / socket.io protocol layer, not Fluid protocol layer.
We might need to have special treatment for server_disconnect for other reasons in addition to it, but currently I'm not sure it's needed.
Thoughts?


Refers to: packages/drivers/odsp-socket-storage/src/OdspDocumentDeltaConnection.ts:123 in 99bc602. [](commit_id = 99bc602, deletion_comment = False)

@GaryWilber
Copy link
Contributor Author

removeSocketIoReference will handle the socket being closed for any reason, it has a check for !socketReference.socket.connected.
The server_disconnect is kinda of special because the close is doing stuff before learning that the socket is closed (socketReference.socket.connected is true right after server_disconnect is processed).

But something interesting here is that the connect document flow doesn't have a listener for "disconnect". It should.

@vladsud
Copy link
Contributor

vladsud commented Nov 22, 2019

Scenario:

  1. Last user leaves socket. removeSocketIoReference is called, but we want to preserve socket for reuse for next 2 seconds
  2. Connection is lost
  3. Within 2 seconds from Update non-issues GitHub urls #1, socket is reused.

I think you are saying #1 solves the problem, which means (if it does), we do not support that scenario (there is no 2 second window for reuse).
If we have 2 second window (which I assume we do want to keep), then any reason for #2 should be handled properly, whether we hear from server or not.


In reply to: 557638013 [](ancestors = 557638013)

@GaryWilber GaryWilber merged commit 84700b7 into microsoft:release/0.11 Nov 22, 2019
@GaryWilber GaryWilber deleted the user/garywilb/socket_reuse_fix branch November 22, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants