Clear receive timeout when close browser and Deno channels (#1016)

bigmontz · web-flow · commit 84d6f66b78f1 · 2022-11-18T14:15:58.000+01:00
The receive timeout was not being clearer when channel get closed.
With the timeout running, receive timeouts events can still be notified to the existing observers.
This was causing some tests failing and possible issues in production code.

This bug was found in the test `package/neo4j-driver/test/result.test.js (should handle missing onCompleted)`. This test was calling `done` twice because of a late failing coming to the observer.

Node channels are not affected by this error since its timeout implementation is controlled to the socket.
diff --git a/packages/bolt-connection/src/channel/browser/browser-channel.js b/packages/bolt-connection/src/channel/browser/browser-channel.js
@@ -168,6 +168,7 @@ export default class WebSocketChannel {
     return new Promise((resolve, reject) => {
       if (this._ws && this._ws.readyState !== WS_CLOSED) {
         this._open = false
+        this.stopReceiveTimeout()
         this._clearConnectionTimeout()
         this._ws.onclose = () => resolve()
         this._ws.close()
@@ -206,7 +207,7 @@ export default class WebSocketChannel {
    * Start the receive timeout for the channel.
    */
   startReceiveTimeout () {
-    if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
+    if (this._open && this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
       this._receiveTimeoutStarted = true
       this._resetTimeout()
     }
diff --git a/packages/bolt-connection/src/channel/deno/deno-channel.js b/packages/bolt-connection/src/channel/deno/deno-channel.js
@@ -150,6 +150,7 @@ export default class DenoChannel {
   async close () {
     if (this._open) {
       this._open = false 
+      this.stopReceiveTimeout()
       if (this._conn != null) {
         await this._conn.close()
       }
@@ -185,7 +186,7 @@ export default class DenoChannel {
    * Start the receive timeout for the channel.
    */
   startReceiveTimeout () {
-    if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
+    if (this._open && this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
       this._receiveTimeoutStarted = true
       this._resetTimeout()
     }
diff --git a/packages/bolt-connection/test/channel/browser/browser-channel.test.js b/packages/bolt-connection/test/channel/browser/browser-channel.test.js
@@ -412,6 +412,31 @@ describe('WebSocketChannel', () => {
         expect(fakeSetTimeout.clearedTimeouts).toEqual([])
       })
 
+      it('should be cleared when connection closes', async () => {
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.calls[0][1]).toEqual(receiveTimeout)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+
+        await webSocketChannel.close()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(1)
+        expect(fakeSetTimeout.calls.length).toEqual(1)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([0])
+      })
+
+      it('should call not setTimeout(receiveTimeout) when connection is closed', async () => {
+        await webSocketChannel.close()
+
+        webSocketChannel.startReceiveTimeout()
+
+        expect(fakeSetTimeout._timeoutIdCounter).toEqual(0)
+        expect(fakeSetTimeout.calls.length).toEqual(0)
+        expect(fakeSetTimeout.clearedTimeouts).toEqual([])
+      })
+
       it('should call setTimeout(receiveTimeout) after stopped', () => {
         webSocketChannel.startReceiveTimeout()
 
diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/channel/browser/browser-channel.js b/packages/neo4j-driver-deno/lib/bolt-connection/channel/browser/browser-channel.js
@@ -168,6 +168,7 @@ export default class WebSocketChannel {
     return new Promise((resolve, reject) => {
       if (this._ws && this._ws.readyState !== WS_CLOSED) {
         this._open = false
+        this.stopReceiveTimeout()
         this._clearConnectionTimeout()
         this._ws.onclose = () => resolve()
         this._ws.close()
@@ -206,7 +207,7 @@ export default class WebSocketChannel {
    * Start the receive timeout for the channel.
    */
   startReceiveTimeout () {
-    if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
+    if (this._open && this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
       this._receiveTimeoutStarted = true
       this._resetTimeout()
     }
diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/channel/deno/deno-channel.js b/packages/neo4j-driver-deno/lib/bolt-connection/channel/deno/deno-channel.js
@@ -150,6 +150,7 @@ export default class DenoChannel {
   async close () {
     if (this._open) {
       this._open = false 
+      this.stopReceiveTimeout()
       if (this._conn != null) {
         await this._conn.close()
       }
@@ -185,7 +186,7 @@ export default class DenoChannel {
    * Start the receive timeout for the channel.
    */
   startReceiveTimeout () {
-    if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
+    if (this._open && this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
       this._receiveTimeoutStarted = true
       this._resetTimeout()
     }
diff --git a/packages/neo4j-driver/test/temporal-types.test.js b/packages/neo4j-driver/test/temporal-types.test.js
@@ -20,7 +20,6 @@
 import neo4j from '../src'
 import sharedNeo4j from './internal/shared-neo4j'
 import { toNumber, internal } from 'neo4j-driver-core'
-import timesSeries from 'async/timesSeries'
 import testUtils from './internal/test-utils'
 
 const {
@@ -55,11 +54,11 @@ describe('#integration temporal-types', () => {
 
   beforeAll(() => {
     driver = neo4j.driver(
-      `bolt://${sharedNeo4j.hostname}`,
+      `${sharedNeo4j.scheme}://${sharedNeo4j.hostname}`,
       sharedNeo4j.authToken
     )
     driverWithNativeNumbers = neo4j.driver(
-      `bolt://${sharedNeo4j.hostname}`,
+      `${sharedNeo4j.scheme}://${sharedNeo4j.hostname}`,
       sharedNeo4j.authToken,
       { disableLosslessIntegers: true }
     )
@@ -101,13 +100,13 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random Duration', async () => {
+  describe('Duration', () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() => randomDuration())
-  }, 90000)
+    testSendAndReceiveRandomTemporalValues('Duration', () => randomDuration())
+  })
 
   it('should send and receive Duration when disableLosslessIntegers=true', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -169,13 +168,13 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random LocalTime', async () => {
+  describe('LocalTime', () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() => randomLocalTime())
-  }, 90000)
+    testSendAndReceiveRandomTemporalValues('LocalTime', () => randomLocalTime())
+  })
 
   it('should send and receive array of LocalTime', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -226,13 +225,13 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random Time', async () => {
+  describe('Time', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() => randomTime())
-  }, 90000)
+    testSendAndReceiveRandomTemporalValues('Time', () => randomTime())
+  })
 
   it('should send and receive array of Time', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -281,13 +280,13 @@ describe('#integration temporal-types', () => {
     await testSendReceiveTemporalValue(new neo4j.types.Date(1923, 8, 14))
   }, 90000)
 
-  it('should send and receive random Date', async () => {
+  describe('Date', () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() => randomDate())
-  }, 90000)
+    testSendAndReceiveRandomTemporalValues('Date', () => randomDate())
+  })
 
   it('should send and receive array of Date', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -346,13 +345,13 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random LocalDateTime', async () => {
+  describe('LocalDateTime', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() => randomLocalDateTime())
-  }, 90000)
+    testSendAndReceiveRandomTemporalValues('LocalDateTime', () => randomLocalDateTime())
+  })
 
   it('should send and receive array of random LocalDateTime', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -442,15 +441,15 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random DateTime with zone offset', async () => {
+  describe('DateTime with zone offset', () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() =>
+    testSendAndReceiveRandomTemporalValues('DateTime with zone offset', () =>
       randomDateTimeWithZoneOffset()
     )
-  }, 90000)
+  })
 
   it('should send and receive array of DateTime with zone offset', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -540,15 +539,15 @@ describe('#integration temporal-types', () => {
     )
   }, 90000)
 
-  it('should send and receive random DateTime with zone id', async () => {
+  describe('DateTime with zone id', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
       return
     }
 
-    await testSendAndReceiveRandomTemporalValues(() =>
+    testSendAndReceiveRandomTemporalValues('DateTime with zone id', () =>
       randomDateTimeWithZoneId()
     )
-  }, 90000)
+  })
 
   it('should send and receive array of DateTime with zone id', async () => {
     if (neo4jDoesNotSupportTemporalTypes()) {
@@ -1403,22 +1402,12 @@ describe('#integration temporal-types', () => {
     )
   })
 
-  function testSendAndReceiveRandomTemporalValues (valueGenerator) {
-    const asyncFunction = (index, callback) => {
-      testSendReceiveTemporalValue(valueGenerator())
-        .then(() => callback())
-        .catch(error => callback(error))
-    }
-
-    return new Promise((resolve, reject) => {
-      timesSeries(RANDOM_VALUES_TO_TEST, asyncFunction, (error, result) => {
-        if (error) {
-          reject(error)
-        } else {
-          resolve(result)
-        }
+  function testSendAndReceiveRandomTemporalValues (temporalType, valueGenerator) {
+    for (let i = 0; i < RANDOM_VALUES_TO_TEST; i++) {
+      it(`should send and receive random ${temporalType} [index=${i}]`, async () => {
+        await testSendReceiveTemporalValue(valueGenerator())
       })
-    })
+    }
   }
 
   async function testSendAndReceiveArrayOfRandomTemporalValues (valueGenerator) {
@@ -1463,10 +1452,10 @@ describe('#integration temporal-types', () => {
   }
 
   async function testSendReceiveTemporalValue (value) {
-    const result = await session.run(
+    const result = await session.executeWrite(tx => tx.run(
       'CREATE (n:Node {value: $value}) RETURN n.value',
       { value: value }
-    )
+    ))
 
     const records = result.records
     expect(records.length).toEqual(1)
