From e1dc4b55e49fcff9e9d8f9891f258b20ca8fbd54 Mon Sep 17 00:00:00 2001 From: Paulo Lanzarin <4529051+prlanzarin@users.noreply.github.com> Date: Mon, 22 Jan 2024 13:10:41 -0300 Subject: [PATCH] fix(bbb-html5): customHeartbeat would not close stale sessions, + (#19017) * fix(bbb-html5): customHeartbeat would not close stale sessions, + The [disabled by default] custom heartbeat included in Meteor's server does not end connections when they are considered unhealthy/stale, which deviates a bit from the default implementation. See: https://github.com/bigbluebutton/bigbluebutton/pull/11486. This commit includes a call to the default heartbeat termination timeout so sockets are correctly cleaned up when the custom heartbeat is activated. It also adds a customHeartbeatUseDataFrames config to allow controlling whether the custom heartbeat should use WS data frames as valid heartbeats as well - this should only be useful for testing/debugging purposes and the default behavior (true) is maintained. As a side note: this change spun off from an investigation where some problematic networks were triggering periodic client re-connects due to the default heartbeat failing. Investigation points to the control frames being put alongside fragmented WS data frames and the server side failing to recognize the former - which means pong frames would be missed and the health check would fail. Since the default heartbeat _does not_ account for data frame traffic (eg DDP payloads), it would shut down the client's WS even though it was healthy. The custom heartbeat _does_ account for data frames, which mitigates that scenario and prevents unecessary reconnections. * fix(bbb-html5): frontend crash due to undefined vars in customHeartbeat Meteor frontends may crash when customHeartbeat is enabled due to an undefined access in the heartbeat`s logger. Add optional chaining to the session props access so it won`t crash and tune down some log levels around that area. --- .../imports/startup/server/index.js | 73 +++++++++++-------- .../private/config/settings.yml | 1 + 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/bigbluebutton-html5/imports/startup/server/index.js b/bigbluebutton-html5/imports/startup/server/index.js index 67f572374d..00c3718417 100755 --- a/bigbluebutton-html5/imports/startup/server/index.js +++ b/bigbluebutton-html5/imports/startup/server/index.js @@ -66,40 +66,51 @@ Meteor.startup(() => { }, healthCheckInterval); } - const { customHeartbeat } = APP_CONFIG; + const { customHeartbeat, customHeartbeatUseDataFrames } = APP_CONFIG; if (customHeartbeat) { Logger.warn('Custom heartbeat functions are enabled'); // https://github.com/sockjs/sockjs-node/blob/1ef08901f045aae7b4df0f91ef598d7a11e82897/lib/transport/websocket.js#L74-L82 - const newHeartbeat = function heartbeat() { - const currentTime = new Date().getTime(); + const heartbeatFactory = function ({ heartbeatTimeoutCallback }) { + return function () { + const currentTime = new Date().getTime(); - // Skipping heartbeat, because websocket is sending data - if (currentTime - this.ws.lastSentFrameTimestamp < 10000) { - try { - Logger.info('Skipping heartbeat, because websocket is sending data', { - currentTime, - lastSentFrameTimestamp: this.ws.lastSentFrameTimestamp, - userId: this.session.connection._meteorSession.userId, - }); - return; - } catch (err) { - Logger.error(`Skipping heartbeat error: ${err}`); - } - } - - const supportsHeartbeats = this.ws.ping(null, () => clearTimeout(this.hto_ref)); - if (supportsHeartbeats) { - this.hto_ref = setTimeout(() => { - try { - Logger.info('Heartbeat timeout', { userId: this.session.connection._meteorSession.userId, sentAt: currentTime, now: new Date().getTime() }); - } catch (err) { - Logger.error(`Heartbeat timeout error: ${err}`); + if (customHeartbeatUseDataFrames) { + // Skipping heartbeat, because websocket is sending data + if (currentTime - this.ws.lastSentFrameTimestamp < 10000) { + try { + Logger.debug('Skipping heartbeat, because websocket is sending data', { + currentTime, + lastSentFrameTimestamp: this.ws.lastSentFrameTimestamp, + userId: this.session?.connection?._meteorSession?.userId, + }); + return; + } catch (err) { + Logger.error(`Skipping heartbeat error: ${err}`); + } } - }, Meteor.server.options.heartbeatTimeout); - } else { - Logger.error('Unexpected error supportsHeartbeats=false'); - } + } + + const supportsHeartbeats = this.ws.ping(null, () => { + clearTimeout(this.hto_ref); + }); + + if (supportsHeartbeats) { + this.hto_ref = setTimeout(() => { + try { + Logger.warn('Heartbeat timeout', { userId: this.session?.connection?._meteorSession?.userId, sentAt: currentTime, now: new Date().getTime() }); + } catch (err) { + Logger.error(`Heartbeat timeout error: ${err}`); + } finally { + if (typeof heartbeatTimeoutCallback === 'function') { + heartbeatTimeoutCallback(); + } + } + }, Meteor.server.options.heartbeatTimeout); + } else { + Logger.error('Unexpected error supportsHeartbeats=false'); + } + }; }; // https://github.com/davhani/hagty/blob/6a5c78e9ae5a5e4ade03e747fb4cc8ea2df4be0c/faye-websocket/lib/faye/websocket/api.js#L84-L88 @@ -134,8 +145,10 @@ Meteor.startup(() => { } recv.ws.meteorHeartbeat = session.heartbeat; - recv.__proto__.heartbeat = newHeartbeat; - recv.ws.__proto__.send = newSend; + recv.heartbeat = heartbeatFactory({ + heartbeatTimeoutCallback: recv.heartbeat_cb + }); + recv.ws.send = newSend; session.bbbFixApplied = true; } }, 5000); diff --git a/bigbluebutton-html5/private/config/settings.yml b/bigbluebutton-html5/private/config/settings.yml index 02acfded33..f8647ad7c8 100755 --- a/bigbluebutton-html5/private/config/settings.yml +++ b/bigbluebutton-html5/private/config/settings.yml @@ -136,6 +136,7 @@ public: breakoutRoomLimit: 16 # https://github.com/bigbluebutton/bigbluebutton/pull/10826 customHeartbeat: false + customHeartbeatUseDataFrames: true showAllAvailableLocales: true # Show "Audio Filters for Microphone" option in settings menu. # When set to true, users are able to enable/disable microphone constraints,