Add support for connection.recv_timeout_seconds in the Browser Channel (#955)

bigmontz · web-flow · commit 5294b99a8dc1 · 2022-07-04T12:55:13.000+02:00
The implementation of the timeout connection hint has changed. The control of when start and stop the timeout has moved to the `channel-channel` instead of relying on the socket timeout configuration.

This change enables the `browser-channel` to implement the `receive timeout` which was not possible before since the `WebSocket` doesn't have a timeout configuration.
diff --git a/packages/bolt-connection/src/channel/browser/browser-channel.js b/packages/bolt-connection/src/channel/browser/browser-channel.js
@@ -53,6 +53,9 @@ export default class WebSocketChannel {
     this._error = null
     this._handleConnectionError = this._handleConnectionError.bind(this)
     this._config = config
+    this._receiveTimeout = null
+    this._receiveTimeoutStarted = false
+    this._receiveTimeoutId = null
 
     const { scheme, error } = determineWebSocketScheme(config, protocolSupplier)
     if (error) {
@@ -84,6 +87,7 @@ export default class WebSocketChannel {
       }
     }
     this._ws.onmessage = event => {
+      this._resetTimeout()
       if (self.onmessage) {
         const b = new ChannelBuffer(event.data)
         self.onmessage(b)
@@ -111,7 +115,7 @@ export default class WebSocketChannel {
     }
 
     // onerror triggers on websocket close as well.. don't get me started.
-    if (this._open) {
+    if (this._open && !this._timedout) {
       // https://stackoverflow.com/questions/25779831/how-to-catch-websocket-connection-to-ws-xxxnn-failed-connection-closed-be
       this._error = newError(
         'WebSocket connection failure. Due to security ' +
@@ -181,18 +185,56 @@ export default class WebSocketChannel {
    * @param {number} receiveTimeout The amount of time the channel will keep without receive any data before timeout (ms)
    * @returns {void}
    */
-  setupReceiveTimeout (receiveTimeout) {}
+  setupReceiveTimeout (receiveTimeout) {
+    this._receiveTimeout = receiveTimeout
+  }
 
   /**
    * Stops the receive timeout for the channel.
    */
   stopReceiveTimeout () {
+    if (this._receiveTimeout !== null && this._receiveTimeoutStarted) {
+      this._receiveTimeoutStarted = false
+      if (this._receiveTimeoutId != null) {
+        clearTimeout(this._receiveTimeoutId)
+      }
+      this._receiveTimeoutId = null
+    }
   }
 
   /**
    * Start the receive timeout for the channel.
    */
   startReceiveTimeout () {
+    if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
+      this._receiveTimeoutStarted = true
+      this._resetTimeout()
+    }
+  }
+
+  _resetTimeout () {
+    if (!this._receiveTimeoutStarted) {
+      return
+    }
+
+    if (this._receiveTimeoutId !== null) {
+      clearTimeout(this._receiveTimeoutId)
+    }
+
+    this._receiveTimeoutId = setTimeout(() => {
+      this._receiveTimeoutId = null
+      this._timedout = true
+      this.stopReceiveTimeout()
+      this._error = newError(
+        `Connection lost. Server didn't respond in ${this._receiveTimeout}ms`,
+        this._config.connectionErrorCode
+      )
+
+      this.close()
+      if (this.onerror) {
+        this.onerror(this._error)
+      }
+    }, this._receiveTimeout)
   }
 
   /**
diff --git a/packages/bolt-connection/test/channel/browser/browser-channel.test.js b/packages/bolt-connection/test/channel/browser/browser-channel.test.js
@@ -19,7 +19,7 @@
 
 import WebSocketChannel from '../../../src/channel/browser/browser-channel'
 import ChannelConfig from '../../../src/channel/channel-config'
-import { error, internal } from 'neo4j-driver-core'
+import { error, internal, newError } from 'neo4j-driver-core'
 import { setTimeoutMock } from '../../timers-util'
 
 const {
@@ -360,6 +360,230 @@ describe('WebSocketChannel', () => {
     })
   })
 
+  describe('.startReceiveTimeout()', () => {
+    let fakeSetTimeout
+    beforeEach(() => {
+      const address = ServerAddress.fromUrl('https://localhost:8989')
+      const channelConfig = new ChannelConfig(
+        address,
+        { connectionTimeout: 0 },
+        SERVICE_UNAVAILABLE
+      )
+      webSocketChannel = new WebSocketChannel(
+        channelConfig,
+        undefined,
+        createWebSocketFactory(WS_OPEN)
+      )
+      fakeSetTimeout = setTimeoutMock.install()
+      fakeSetTimeout.pause()
+    })
+
+    afterEach(() => {
+      fakeSetTimeout.uninstall()
+    })
+
+    describe('receive timeout is setup', () => {
+      const receiveTimeout = 1000
+      beforeEach(() => {
+        webSocketChannel.setupReceiveTimeout(receiveTimeout)
+      })
+
+      it('should call setTimeout(receiveTimeout) when it call first', () => {
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+      })
+
+      it('should call not setTimeout(receiveTimeout) when already started', () => {
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+      })
+
+      it('should call setTimeout(receiveTimeout) after stopped', () => {
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+
+        webSocketChannel.stopReceiveTimeout()
+
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(2)
+        expect(fakeSetTimeout.calls.length).toEqual(2)
+        expect(fakeSetTimeout.calls[1][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([0])
+      })
+
+      describe('on times out', () => {
+        beforeEach(() => {
+          webSocketChannel.startReceiveTimeout()
+        })
+
+        it('should notify the error', () => {
+          webSocketChannel.onerror = jest.fn()
+
+          fakeSetTimeout.calls[0][0]()
+
+          expect(webSocketChannel.onerror).toBeCalledWith(newError(
+            'Connection lost. Server didn\'t respond in 1000ms',
+            webSocketChannel._config.connectionErrorCode
+          ))
+        })
+
+        it('should close the connection', () => {
+          fakeSetTimeout.calls[0][0]()
+
+          expect(webSocketChannel._ws.readyState).toBe(WS_CLOSED)
+        })
+
+        it('should close the timedout', () => {
+          fakeSetTimeout.calls[0][0]()
+
+          expect(webSocketChannel._timedout).toBe(true)
+        })
+
+        describe('onmessage', () => {
+          it('should not reset the timer', () => {
+            fakeSetTimeout.calls[0][0]()
+
+            webSocketChannel._ws.onmessage('')
+
+            expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+            expect(fakeSetTimeout.calls.length).toEqual(1)
+          })
+        })
+      })
+
+      describe('onmessage', () => {
+        it('should reset the timer', () => {
+          webSocketChannel.startReceiveTimeout()
+
+          expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+          expect(fakeSetTimeout.calls.length).toEqual(1)
+          expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+          expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+
+          webSocketChannel._ws.onmessage('')
+
+          expect(fakeSetTimeout._timeoutIdCounter).toEqual(2)
+          expect(fakeSetTimeout.calls.length).toEqual(2)
+          expect(fakeSetTimeout.calls[1][1]).toEqual(receiveTimeout)
+          expect(fakeSetTimeout.clearedTimeouts).toEqual([0])
+        })
+      })
+    })
+
+    describe('receive timeout is not setup', () => {
+      it('should not call setTimeout(receiveTimeout) when not configured', () => {
+        // start
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(0)
+      })
+
+      describe('onmessage', () => {
+        it('should reset the timer', () => {
+          webSocketChannel.startReceiveTimeout()
+
+          webSocketChannel._ws.onmessage('')
+
+          expect(fakeSetTimeout._timeoutIdCounter).toEqual(0)
+          expect(fakeSetTimeout.calls.length).toEqual(0)
+        })
+      })
+    })
+  })
+
+  describe('.stopReceiveTimeout()', () => {
+    let fakeSetTimeout
+    beforeEach(() => {
+      const address = ServerAddress.fromUrl('https://localhost:8989')
+      const channelConfig = new ChannelConfig(
+        address,
+        { connectionTimeout: 0 },
+        SERVICE_UNAVAILABLE
+      )
+      webSocketChannel = new WebSocketChannel(
+        channelConfig,
+        undefined,
+        createWebSocketFactory(WS_OPEN)
+      )
+      fakeSetTimeout = setTimeoutMock.install()
+      fakeSetTimeout.pause()
+    })
+
+    afterEach(() => {
+      fakeSetTimeout.uninstall()
+    })
+
+    describe('receive timeout is setup', () => {
+      const receiveTimeout = 1000
+      beforeEach(() => {
+        webSocketChannel.setupReceiveTimeout(receiveTimeout)
+      })
+
+      it('should not clear timeout when it is not started', () => {
+        webSocketChannel.stopReceiveTimeout()
+
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+      })
+
+      it('should clear timeout when it is started', () => {
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+
+        webSocketChannel.stopReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([0])
+      })
+
+      describe('onmessage', () => {
+        it('should reset the timer when stoped', () => {
+          webSocketChannel.startReceiveTimeout()
+          webSocketChannel.stopReceiveTimeout()
+
+          webSocketChannel._ws.onmessage('')
+
+          expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+          expect(fakeSetTimeout.calls.length).toEqual(1)
+        })
+      })
+    })
+
+    describe('receive timeout is not setup', () => {
+      it('should not call clearTimeout() when not configured', () => {
+        // start
+        webSocketChannel.startReceiveTimeout()
+        webSocketChannel.stopReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(0)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+      })
+    })
+  })
+
   function createWebSocketFactory (readyState) {
     const ws = {}
     ws.readyState = readyState
diff --git a/packages/bolt-connection/test/timers-util.js b/packages/bolt-connection/test/timers-util.js
@@ -28,6 +28,7 @@ class SetTimeoutMock {
         code()
         this.invocationDelays.push(delay)
       }
+      this.calls.push([...arguments])
       return this._timeoutIdCounter++
     }
 
@@ -59,6 +60,7 @@ class SetTimeoutMock {
     this._paused = false
     this._timeoutIdCounter = 0
 
+    this.calls = []
     this.invocationDelays = []
     this.clearedTimeouts = []
   }

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ class SetTimeoutMock {`
`28`	`28`	`code()`
`29`	`29`	`this.invocationDelays.push(delay)`
`30`	`30`	`}`
	`31`	`+ this.calls.push([...arguments])`
`31`	`32`	`return this._timeoutIdCounter++`
`32`	`33`	`}`
`33`	`34`
`@@ -59,6 +60,7 @@ class SetTimeoutMock {`
`59`	`60`	`this._paused = false`
`60`	`61`	`this._timeoutIdCounter = 0`
`61`	`62`
	`63`	`+ this.calls = []`
`62`	`64`	`this.invocationDelays = []`
`63`	`65`	`this.clearedTimeouts = []`
`64`	`66`	`}`