From 7df0ce5570832b4f1ef2208f1a67cf61ca266d97 Mon Sep 17 00:00:00 2001 From: tsm-odoo Date: Fri, 10 Feb 2023 09:30:33 +0000 Subject: [PATCH] [FIX] bus: do not handle events of outdated websockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, late close events were not handled properly. This could have led to non-reconnecting websockets. The problematic scheme is the following: - Close the socket (eg. upon the reception of an `offline` event), let's assume that the other end will not perform the closing handshake, the connection will be closed once the browser presumes it is dead. - Create a new socket (eg. upon the reception of an `online` event) - The browser assumes the connection is dead and dispatches a `close` event, the worker switches to the `reconnecting` state and expects an `open` event to update its state to connected. Since there is already a running socket, the worker won't open a new one and will never receive the `open` event. - Server closes the connection (eg. `KEEP_ALIVE_TIMEOUT`) - The close handler is called but since it is in the `reconnecting` state, it assumes it shouldn't do anything thus, no reconnect attempt is made. This PR fixed this issue by ignoring events linked to outdated sockets. closes odoo/odoo#112456 X-original-commit: a2454739156d8742f7606f090126f32735fb15df Signed-off-by: Sébastien Theys (seb) --- .../static/src/workers/websocket_worker.js | 37 ++++++++-- addons/bus/static/tests/bus_tests.js | 67 +++++++++++++++++++ .../bus/static/tests/helpers/mock_server.js | 4 +- 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/addons/bus/static/src/workers/websocket_worker.js b/addons/bus/static/src/workers/websocket_worker.js index e58a697619e..200869c4c57 100644 --- a/addons/bus/static/src/workers/websocket_worker.js +++ b/addons/bus/static/src/workers/websocket_worker.js @@ -34,7 +34,7 @@ export const WEBSOCKET_CLOSE_CODES = Object.freeze({ }); // Should be incremented on every worker update in order to force // update of the worker in browser cache. -export const WORKER_VERSION = '1.0.3'; +export const WORKER_VERSION = '1.0.4'; const INITIAL_RECONNECT_DELAY = 1000; const MAXIMUM_RECONNECT_DELAY = 60000; @@ -60,6 +60,11 @@ export class WebsocketWorker { this.lastNotificationId = 0; this.messageWaitQueue = []; this._forceUpdateChannels = debounce(this._forceUpdateChannels, 300, true); + + this._onWebsocketClose = this._onWebsocketClose.bind(this); + this._onWebsocketError = this._onWebsocketError.bind(this); + this._onWebsocketMessage = this._onWebsocketMessage.bind(this); + this._onWebsocketOpen = this._onWebsocketOpen.bind(this); } //-------------------------------------------------------------------------- @@ -250,6 +255,16 @@ export class WebsocketWorker { return this.websocket && this.websocket.readyState === 0; } + /** + * Determine whether or not the websocket associated to this worker + * is in the closing state. + * + * @returns {boolean} + */ + _isWebsocketClosing() { + return this.websocket && this.websocket.readyState === 2; + } + /** * Triggered when a connection is closed. If closure was not clean , * try to reconnect after indicating to the clients that the @@ -363,11 +378,23 @@ export class WebsocketWorker { if (this._isWebsocketConnected() || this._isWebsocketConnecting()) { return; } + if (this.websocket) { + this.websocket.removeEventListener('open', this._onWebsocketOpen); + this.websocket.removeEventListener('message', this._onWebsocketMessage); + this.websocket.removeEventListener('error', this._onWebsocketError); + this.websocket.removeEventListener('close', this._onWebsocketClose); + } + if (this._isWebsocketClosing()) { + // close event was not triggered and will never be, broadcast the + // disconnect event for consistency sake. + this.lastChannelSubscription = null; + this.broadcast("disconnect", { code: WEBSOCKET_CLOSE_CODES.ABNORMAL_CLOSURE }); + } this.websocket = new WebSocket(this.websocketURL); - this.websocket.addEventListener('open', this._onWebsocketOpen.bind(this)); - this.websocket.addEventListener('error', this._onWebsocketError.bind(this)); - this.websocket.addEventListener('message', this._onWebsocketMessage.bind(this)); - this.websocket.addEventListener('close', this._onWebsocketClose.bind(this)); + this.websocket.addEventListener('open', this._onWebsocketOpen); + this.websocket.addEventListener('error', this._onWebsocketError); + this.websocket.addEventListener('message', this._onWebsocketMessage); + this.websocket.addEventListener('close', this._onWebsocketClose); } /** diff --git a/addons/bus/static/tests/bus_tests.js b/addons/bus/static/tests/bus_tests.js index 47c20494afb..4d162754028 100644 --- a/addons/bus/static/tests/bus_tests.js +++ b/addons/bus/static/tests/bus_tests.js @@ -480,6 +480,73 @@ QUnit.module('Bus', { await nextTick(); assert.verifySteps([]); }); + + QUnit.test("Can reconnect after late close event", async function (assert) { + let subscribeSent = 0; + const closeDeferred = makeDeferred(); + let openDeferred = makeDeferred(); + const worker = patchWebsocketWorkerWithCleanup({ + _onWebsocketOpen() { + this._super(); + openDeferred.resolve(); + }, + _sendToServer({ event_name }) { + if (event_name === "subscribe") { + subscribeSent++; + } + }, + }); + const pyEnv = await startServer(); + const env = await makeTestEnv(); + env.services["bus_service"].start(); + await openDeferred; + patchWithCleanup(worker.websocket, { + close(code = WEBSOCKET_CLOSE_CODES.CLEAN, reason) { + this.readyState = 2; + const _super = this._super; + if (code === WEBSOCKET_CLOSE_CODES.CLEAN) { + closeDeferred.then(() => { + // Simulate that the connection could not be closed cleanly. + _super(WEBSOCKET_CLOSE_CODES.ABNORMAL_CLOSURE, reason); + }); + } else { + _super(code, reason); + } + }, + }); + env.services["bus_service"].addEventListener("connect", () => assert.step("connect")); + env.services["bus_service"].addEventListener("disconnect", () => assert.step("disconnect")); + env.services["bus_service"].addEventListener("reconnecting", () => assert.step("reconnecting")); + env.services["bus_service"].addEventListener("reconnect", () => assert.step("reconnect")); + // Connection will be closed when passing offline. But the close event + // will be delayed to come after the next open event. The connection + // will thus be in the closing state in the meantime. + window.dispatchEvent(new Event("offline")); + await nextTick(); + openDeferred = makeDeferred(); + // Worker reconnects upon the reception of the online event. + window.dispatchEvent(new Event("online")); + await openDeferred; + closeDeferred.resolve(); + // Trigger the close event, it shouldn't have any effect since it is + // related to an old connection that is no longer in use. + await nextTick(); + openDeferred = makeDeferred(); + // Server closes the connection, the worker should reconnect. + pyEnv.simulateConnectionLost(WEBSOCKET_CLOSE_CODES.KEEP_ALIVE_TIMEOUT); + await openDeferred; + await nextTick(); + // 3 connections were opened, so 3 subscriptions are expected. + assert.strictEqual(subscribeSent, 3); + assert.verifySteps([ + "connect", + "disconnect", + "connect", + "disconnect", + "reconnecting", + "reconnect", + ]); + }); }); }); diff --git a/addons/bus/static/tests/helpers/mock_server.js b/addons/bus/static/tests/helpers/mock_server.js index 4872fd2ed08..5bc3ba62579 100644 --- a/addons/bus/static/tests/helpers/mock_server.js +++ b/addons/bus/static/tests/helpers/mock_server.js @@ -74,8 +74,6 @@ patch(MockServer.prototype, 'bus', { * @param {number} clodeCode the code to close the connection with. */ _simulateConnectionLost(closeCode) { - this.websocketWorker.websocket.dispatchEvent(new CloseEvent('close', { - code: closeCode, - })); + this.websocketWorker.websocket.close(closeCode); }, });