-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
Support HTTP long polling as an alternative to WebSockets #859
Changes from 7 commits
72c619f
ceb5c78
0e5ea20
e7e060c
774b9a4
a354b9b
ea08b49
1961b2c
adcdd5f
80a1b3a
07818ff
8af0eaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
import WS from 'ws'; | ||
import {Socket as EIOSocket} from 'engine.io-client'; | ||
|
||
export interface GristClientSocketOptions { | ||
headers?: Record<string, string>; | ||
} | ||
|
||
export class GristClientSocket { | ||
private _wsSocket: WS.WebSocket | WebSocket | undefined; | ||
private _eioSocket: EIOSocket | undefined; | ||
|
||
// Set to true when the connection process is complete, either succesfully or | ||
// after the WebSocket and polling transports have both failed. Events from | ||
// the underlying socket are not forwarded to the client until that point. | ||
private _openDone: boolean = false; | ||
|
||
private _messageHandler: null | ((data: string) => void); | ||
private _openHandler: null | (() => void); | ||
private _errorHandler: null | ((err: Error) => void); | ||
private _closeHandler: null | (() => void); | ||
|
||
constructor(private _url: string, private _options?: GristClientSocketOptions) { | ||
this._createWSSocket(); | ||
} | ||
|
||
public set onmessage(cb: null | ((data: string) => void)) { | ||
this._messageHandler = cb; | ||
} | ||
|
||
public set onopen(cb: null | (() => void)) { | ||
this._openHandler = cb; | ||
} | ||
|
||
public set onerror(cb: null | ((err: Error) => void)) { | ||
this._errorHandler = cb; | ||
} | ||
|
||
public set onclose(cb: null | (() => void)) { | ||
this._closeHandler = cb; | ||
} | ||
|
||
public close() { | ||
if (this._wsSocket) { | ||
this._wsSocket.close(); | ||
} else { | ||
this._eioSocket!.close(); | ||
} | ||
} | ||
|
||
public send(data: string) { | ||
if (this._wsSocket) { | ||
this._wsSocket.send(data); | ||
} else { | ||
this._eioSocket!.send(data); | ||
} | ||
} | ||
|
||
// pause() and resume() are used for tests and assume a WS.WebSocket transport | ||
public pause() { | ||
(this._wsSocket as WS.WebSocket)?.pause(); | ||
} | ||
|
||
public resume() { | ||
(this._wsSocket as WS.WebSocket)?.resume(); | ||
} | ||
|
||
private _createWSSocket() { | ||
if(typeof WebSocket !== 'undefined') { | ||
this._wsSocket = new WebSocket(this._url); | ||
} else { | ||
this._wsSocket = new WS(this._url, undefined, this._options); | ||
} | ||
this._wsSocket.onmessage = this._onWSMessage.bind(this); | ||
this._wsSocket.onopen = this._onWSOpen.bind(this); | ||
this._wsSocket.onerror = this._onWSError.bind(this); | ||
this._wsSocket.onclose = this._onWSClose.bind(this); | ||
} | ||
|
||
private _destroyWSSocket() { | ||
if (this._wsSocket) { | ||
this._wsSocket.onmessage = null; | ||
this._wsSocket.onopen = null; | ||
this._wsSocket.onerror = null; | ||
this._wsSocket.onclose = null; | ||
this._wsSocket = undefined; | ||
} | ||
} | ||
|
||
private _onWSMessage(event: WS.MessageEvent | MessageEvent<any>) { | ||
if (this._openDone) { | ||
// event.data is guaranteed to be a string here because we only send text frames. | ||
// https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/message_event#event_properties | ||
this._messageHandler?.(event.data); | ||
} | ||
} | ||
|
||
private _onWSOpen() { | ||
// The connection was established successfully. Any future events can now | ||
// be forwarded to the client. | ||
this._openDone = true; | ||
this._openHandler?.(); | ||
} | ||
|
||
private _onWSError(ev: Event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure it's impossible to receive the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I ended up simplifying the logic, replacing |
||
// The first connection attempt failed. Trigger an attempt with another transport. | ||
this._destroyWSSocket(); | ||
this._createEIOSocket(); | ||
} | ||
|
||
private _onWSClose() { | ||
if (this._openDone) { | ||
this._closeHandler?.(); | ||
} | ||
} | ||
|
||
private _createEIOSocket() { | ||
this._eioSocket = new EIOSocket(this._url, { | ||
path: new URL(this._url).pathname, | ||
addTrailingSlash: false, | ||
transports: ['polling'], | ||
upgrade: false, | ||
extraHeaders: this._options?.headers, | ||
withCredentials: true, | ||
}); | ||
|
||
this._eioSocket.on('message', this._onEIOMessage.bind(this)); | ||
this._eioSocket.on('open', this._onEIOOpen.bind(this)); | ||
this._eioSocket.on('error', this._onEIOError.bind(this)); | ||
this._eioSocket.on('close', this._onEIOClose.bind(this)); | ||
} | ||
|
||
private _onEIOMessage(data: string) { | ||
this._messageHandler?.(data); | ||
} | ||
|
||
private _onEIOOpen() { | ||
this._openHandler?.(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned below, I reworked the logic and there's only one state check left, in |
||
} | ||
|
||
private _onEIOError(ev: any) { | ||
// We will make no further attempt to connect. Any future events can now | ||
// be forwarded to the client. | ||
this._openDone = true; | ||
this._errorHandler?.(ev); | ||
} | ||
|
||
private _onEIOClose() { | ||
this._closeHandler?.(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ import {fromCallback} from 'app/server/lib/serverUtils'; | |
import {i18n} from 'i18next'; | ||
import * as crypto from 'crypto'; | ||
import moment from 'moment'; | ||
import * as WebSocket from 'ws'; | ||
import {GristServerSocket} from 'app/server/lib/GristServerSocket'; | ||
|
||
// How many messages and bytes to accumulate for a disconnected client before booting it. | ||
// The benefit is that a client who temporarily disconnects and reconnects without missing much, | ||
|
@@ -97,8 +97,7 @@ export class Client { | |
private _missedMessagesTotalLength: number = 0; | ||
private _destroyTimer: NodeJS.Timer|null = null; | ||
private _destroyed: boolean = false; | ||
private _websocket: WebSocket|null; | ||
private _websocketEventHandlers: Array<{event: string, handler: (...args: any[]) => void}> = []; | ||
private _websocket: GristServerSocket|null; | ||
private _org: string|null = null; | ||
private _profile: UserProfile|null = null; | ||
private _user: FullUser|undefined = undefined; | ||
|
@@ -131,18 +130,14 @@ export class Client { | |
return this._locale; | ||
} | ||
|
||
public setConnection(websocket: WebSocket, counter: string|null, browserSettings: BrowserSettings) { | ||
public setConnection(websocket: GristServerSocket, counter: string|null, browserSettings: BrowserSettings) { | ||
this._websocket = websocket; | ||
this._counter = counter; | ||
this.browserSettings = browserSettings; | ||
|
||
const addHandler = (event: string, handler: (...args: any[]) => void) => { | ||
websocket.on(event, handler); | ||
this._websocketEventHandlers.push({event, handler}); | ||
}; | ||
addHandler('error', (err: unknown) => this._onError(err)); | ||
addHandler('close', () => this._onClose()); | ||
addHandler('message', (msg: string) => this._onMessage(msg)); | ||
websocket.onerror = (err: Error) => this._onError(err); | ||
websocket.onclose = () => this._onClose(); | ||
websocket.onmessage = (msg: string) => this._onMessage(msg); | ||
} | ||
|
||
/** | ||
|
@@ -189,7 +184,7 @@ export class Client { | |
|
||
public interruptConnection() { | ||
if (this._websocket) { | ||
this._removeWebsocketListeners(); | ||
this._websocket?.removeAllListeners(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question mark unneeded here (given the conditional) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, removed. |
||
this._websocket.terminate(); // close() is inadequate when ws routed via loadbalancer | ||
this._websocket = null; | ||
} | ||
|
@@ -359,7 +354,7 @@ export class Client { | |
// See also my report at https://stackoverflow.com/a/48411315/328565 | ||
await delay(250); | ||
|
||
if (!this._destroyed && this._websocket?.readyState === WebSocket.OPEN) { | ||
if (!this._destroyed && this._websocket?.isOpen) { | ||
await this._sendToWebsocket(JSON.stringify({...clientConnectMsg, dup: true})); | ||
} | ||
} catch (err) { | ||
|
@@ -604,7 +599,7 @@ export class Client { | |
/** | ||
* Processes an error on the websocket. | ||
*/ | ||
private _onError(err: unknown) { | ||
private _onError(err: Error) { | ||
this._log.warn(null, "onError", err); | ||
// TODO Make sure that this is followed by onClose when the connection is lost. | ||
} | ||
|
@@ -613,7 +608,7 @@ export class Client { | |
* Processes the closing of a websocket. | ||
*/ | ||
private _onClose() { | ||
this._removeWebsocketListeners(); | ||
this._websocket?.removeAllListeners(); | ||
|
||
// Remove all references to the websocket. | ||
this._websocket = null; | ||
|
@@ -629,15 +624,4 @@ export class Client { | |
this._destroyTimer = setTimeout(() => this.destroy(), Deps.clientRemovalTimeoutMs); | ||
} | ||
} | ||
|
||
private _removeWebsocketListeners() { | ||
if (this._websocket) { | ||
// Avoiding websocket.removeAllListeners() because WebSocket.Server registers listeners | ||
// internally for websockets it keeps track of, and we should not accidentally remove those. | ||
for (const {event, handler} of this._websocketEventHandlers) { | ||
this._websocket.off(event, handler); | ||
} | ||
this._websocketEventHandlers = []; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to have a comment that exactly one of
_wsSocket
,_eioSocket
is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.