From 0c724c4971a1d85d666614e368272e15438d5e8c Mon Sep 17 00:00:00 2001 From: Paul Tagliamonte Date: Thu, 21 Sep 2023 12:07:47 -0400 Subject: [PATCH] Start to restructure the Engine's connection to the backend (#674) * Start to restructure the Engine's connectio to the backend 1) Add in a tearDown stub for when the Engine is torn down. This is now distinct from a 'close', which will not stop connect from trying again. Running tearDown will mark the connection to not be retried and close active connections. 2) Move the retry logic out of connect and into the constructor. It will attempt to reconnect at the same rate as we had previously. 3) The timeout will now only close the connection, not restart it. Signed-off-by: Paul Tagliamonte * Don't continue on dead conn & setTimeout on init only * Clean up extra setTimeout * Keep track of connection timeouts and clear on close * Fix tsc by defining Timeout Signed-off-by: Paul Tagliamonte * appease the format gods --------- Signed-off-by: Paul Tagliamonte Co-authored-by: Adam Sunderland --- src/lang/std/engineConnection.ts | 85 ++++++++++++++++++++++++-------- 1 file changed, 65 insertions(+), 20 deletions(-) diff --git a/src/lang/std/engineConnection.ts b/src/lang/std/engineConnection.ts index 355ab39a5..d89c9b963 100644 --- a/src/lang/std/engineConnection.ts +++ b/src/lang/std/engineConnection.ts @@ -45,6 +45,11 @@ interface NewTrackArgs { mediaStream: MediaStream } +// This looks funny, I know. This is needed because node and the browser +// disagree as to the type. In a browser it's a number, but in node it's a +// "Timeout". +type Timeout = ReturnType + type ClientMetrics = Models['ClientMetrics_type'] // EngineConnection encapsulates the connection(s) to the Engine @@ -56,6 +61,9 @@ export class EngineConnection { unreliableDataChannel?: RTCDataChannel private ready: boolean + private connecting: boolean + private dead: boolean + private failedConnTimeout: Timeout | null readonly url: string private readonly token?: string @@ -91,6 +99,9 @@ export class EngineConnection { this.url = url this.token = token this.ready = false + this.connecting = false + this.dead = false + this.failedConnTimeout = null this.onWebsocketOpen = onWebsocketOpen this.onDataChannelOpen = onDataChannelOpen this.onEngineConnectionOpen = onEngineConnectionOpen @@ -101,7 +112,10 @@ export class EngineConnection { // TODO(paultag): This ought to be tweakable. const pingIntervalMs = 10000 - setInterval(() => { + let pingInterval = setInterval(() => { + if (this.dead) { + clearInterval(pingInterval) + } if (this.isReady()) { // When we're online, every 10 seconds, we'll attempt to put a 'ping' // command through the WebSocket connection. This will help both ends @@ -110,6 +124,24 @@ export class EngineConnection { this.send({ type: 'ping' }) } }, pingIntervalMs) + + const connectionTimeoutMs = VITE_KC_CONNECTION_TIMEOUT_MS + let connectInterval = setInterval(() => { + if (this.dead) { + clearInterval(connectInterval) + return + } + if (this.isReady()) { + return + } + console.log('connecting via retry') + this.connect() + }, connectionTimeoutMs) + } + // isConnecting will return true when connect has been called, but the full + // WebRTC is not online. + isConnecting() { + return this.connecting } // isReady will return true only when the WebRTC *and* WebSocket connection // are connected. During setup, the WebSocket connection comes online first, @@ -118,6 +150,10 @@ export class EngineConnection { isReady() { return this.ready } + tearDown() { + this.dead = true + this.close() + } // shouldTrace will return true when Sentry should be used to instrument // the Engine. shouldTrace() { @@ -129,8 +165,10 @@ export class EngineConnection { // This will attempt the full handshake, and retry if the connection // did not establish. connect() { - // TODO(paultag): make this safe to call multiple times, and figure out - // when a connection is in progress (state: connecting or something). + console.log('connect was called') + if (this.isConnecting() || this.isReady()) { + return + } // Information on the connect transaction @@ -362,20 +400,6 @@ export class EngineConnection { }) }) } - - // TODO(paultag): This ought to be both controllable, as well as something - // like exponential backoff to have some grace on the backend, as well as - // fix responsiveness for clients that had a weird network hiccup. - const connectionTimeoutMs = VITE_KC_CONNECTION_TIMEOUT_MS - - setTimeout(() => { - if (this.isReady()) { - return - } - console.log('engine connection timeout on connection, retrying') - this.close() - this.connect() - }, connectionTimeoutMs) }) this.pc.addEventListener('track', (event) => { @@ -465,6 +489,7 @@ export class EngineConnection { this.onEngineConnectionOpen(this) this.ready = true + this.connecting = false }) this.unreliableDataChannel.addEventListener('close', (event) => { @@ -478,6 +503,22 @@ export class EngineConnection { }) }) + const connectionTimeoutMs = VITE_KC_CONNECTION_TIMEOUT_MS + + if (this.failedConnTimeout) { + console.log('clearing timeout before set') + clearTimeout(this.failedConnTimeout) + this.failedConnTimeout = null + } + console.log('timeout set') + this.failedConnTimeout = setTimeout(() => { + if (this.isReady()) { + return + } + console.log('engine connection timeout on connection, closing') + this.close() + }, connectionTimeoutMs) + this.onConnectionStarted(this) } unreliableSend(message: object | string) { @@ -502,9 +543,15 @@ export class EngineConnection { this.pc = undefined this.unreliableDataChannel = undefined this.webrtcStatsCollector = undefined + if (this.failedConnTimeout) { + console.log('closed timeout in close') + clearTimeout(this.failedConnTimeout) + this.failedConnTimeout = null + } this.onClose(this) this.ready = false + this.connecting = false } } @@ -712,7 +759,7 @@ export class EngineCommandManager { } } tearDown() { - this.engineConnection?.close() + this.engineConnection?.tearDown() } startNewSession() { this.artifactMap = {} @@ -815,7 +862,6 @@ export class EngineCommandManager { lastMessage = command.cmd.type } if (!this.engineConnection?.isReady()) { - console.log('socket not ready') return Promise.resolve() } if (command.type !== 'modeling_cmd_req') return Promise.resolve() @@ -862,7 +908,6 @@ export class EngineCommandManager { this.sourceRangeMap[id] = range if (!this.engineConnection?.isReady()) { - console.log('socket not ready') return Promise.resolve() } this.engineConnection?.send(command)