Add closed flag to prevent latent socket emission #3787
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // We set the closed flag as a part of the contract for overriding the disconnect method. This is used by | ||
| // DocumentDeltaConnection to determine if emitting on the socket is allowed, which is important since | ||
| // OdspDocumentDeltaConnection reuses the socket rather than truly disconnecting it. | ||
| this.closed = true; |
There was a problem hiding this comment.
i don't love this. i'm alway wary of flag as they make the state machine harder to understand. I also don't like having to reach into the shared implementation. Howerver, if we think this will get better soon-ish with refactoring i think it's ok.
There was a problem hiding this comment.
My hope is this all gets simpler with refactoring or BatchManager removal per #3788. Agreed on wanting to minimize state management.
heliocliu
approved these changes
Oct 1, 2020
vladsud
reviewed
Oct 1, 2020
packages/drivers/odsp-driver/src/odspDocumentDeltaConnection.ts
Outdated
Show resolved
Hide resolved
vladsud
approved these changes
Oct 1, 2020
ChumpChief
added a commit
to ChumpChief/FluidFramework
that referenced
this pull request
Oct 1, 2020
Speculative fix to prevent OdspDocumentDeltaConnection from emitting on reused sockets after disconnect.
ChumpChief
added a commit
to ChumpChief/FluidFramework
that referenced
this pull request
Oct 1, 2020
Speculative fix to prevent OdspDocumentDeltaConnection from emitting on reused sockets after disconnect.
ChumpChief
added a commit
that referenced
this pull request
Oct 1, 2020
ChumpChief
added a commit
that referenced
this pull request
Oct 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #3627, we observe messages are being sent with old clientId. The default implementation of
DocumentDeltaManager.disconnect()generally protects against this by disconnecting the socket on disconnect(). However, there's nothing actually stopping theBatchManagerfrom attempting to emit on the old socket if it still has messages in the queue -- it will just fail because the socket is disconnected.This is a speculative fix -- we suspect that since
OdspDocumentDeltaManageroverridesdisconnect()to allow for socket reuse, these still-open sockets are emitting leftover messages. This change introduces a flag to more explicitly indicate/check whether emitting on the socket should be allowed.With Helio's #3704, we should have the correct telemetry in place to determine whether this is effective in eliminating the issue.