Skip to content

Commit

Permalink
Add closed flag to prevent latent socket emission (microsoft#3787)
Browse files Browse the repository at this point in the history
Speculative fix to prevent OdspDocumentDeltaConnection from emitting on reused sockets after disconnect.
  • Loading branch information
ChumpChief committed Oct 1, 2020
1 parent 351a26a commit 7609462
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
17 changes: 15 additions & 2 deletions packages/drivers/driver-base/src/documentDeltaConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,12 @@ export class DocumentDeltaConnection
return !!this._details;
}

/**
* Flag to indicate whether the DocumentDeltaConnection is expected to still be capable of sending messages.
* After disconnection, we flip this to prevent any stale messages from being emitted.
*/
protected closed: boolean = false;

private get details(): IConnected {
if (!this._details) {
throw new Error("Internal error: calling method before _details is initialized!");
Expand All @@ -143,7 +149,12 @@ export class DocumentDeltaConnection

this.submitManager = new BatchManager<IDocumentMessage[]>(
(submitType, work) => {
this.socket.emit(submitType, this.clientId, work);
// Although the implementation here disconnects the socket and does not reuse it, other subclasses
// (e.g. OdspDocumentDeltaConnection) may reuse the socket. In these cases, we need to avoid emitting
// on the still-live socket.
if (!this.closed) {
this.socket.emit(submitType, this.clientId, work);
}
});

this.on("newListener", (event, listener) => {
Expand Down Expand Up @@ -340,13 +351,15 @@ export class DocumentDeltaConnection
}

/**
* Disconnect from the websocket
* Disconnect from the websocket, and permanently disable this DocumentDeltaConnection. Subclasses which
* override this method must set the "closed" flag after disconnecting.
* @param socketProtocolError - true if error happened on socket / socket.io protocol level
* (not on Fluid protocol level)
*/
public disconnect(socketProtocolError: boolean = false) {
this.removeTrackedListeners(false);
this.socket.disconnect();
this.closed = true;
}

protected async initialize(connectMessage: IConnect, timeout: number) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,13 @@ export class OdspDocumentDeltaConnection extends DocumentDeltaConnection impleme
* Disconnect from the websocket
*/
public disconnect(socketProtocolError: boolean = false) {
// 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. Note that below we may
// still send disconnect_document which is allowed; this is only intended to prevent normal messages from
// being emitted.
this.closed = true;

if (this.socketReferenceKey !== undefined) {
const key = this.socketReferenceKey;
this.socketReferenceKey = undefined;
Expand Down

0 comments on commit 7609462

Please sign in to comment.