From 01c9f75aa0e40a57dd82b91cd912a2517bf0fa63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Thu, 23 Nov 2023 11:04:43 +0100 Subject: [PATCH 01/11] Introduce `connectionLivenessCheckTimeout` configuration This configuration defines the number of milliseconds the connection might be idle before need to perform a liveness check on acquiring from the pool. ```typescript const driver = neo4j.driver(URL, AUTH, { // other configurations, then connectionLivenessCheckTimeout: 30000 // 30 seconds }) ``` Check the API docs for me information. --- packages/core/src/types.ts | 28 ++++++++++++++++++++ packages/neo4j-driver-deno/lib/core/types.ts | 28 ++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 1e422ff4f..05e07d134 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -71,6 +71,7 @@ export class Config { maxTransactionRetryTime?: number maxConnectionLifetime?: number connectionAcquisitionTimeout?: number + connectionLivenessCheckTimeout?: number connectionTimeout?: number disableLosslessIntegers?: boolean notificationFilter?: NotificationFilter @@ -184,6 +185,33 @@ export class Config { */ this.maxTransactionRetryTime = 30000 // 30 seconds + /** + * Specify the maximum time in milliseconds the connection can be idle without needing + * to perform a liveness check on acquire from the pool. + * + * Pooled connections that have been idle in the pool for longer than this + * timeout will be tested before they are used again, to ensure they are still live. + * If this option is set too low, an additional network call will be incurred + * when acquiring a connection, which causes a performance hit. + * + * If this is set high, you may receive sessions that are backed by no longer + * live connections, which will lead to exceptions in your application. + * Assuming the database is running, these exceptions will go away if you retry + * acquiring sessions. + * + * Hence, this parameter tunes a balance between the likelihood of your application + * seeing connection problems, and performance. + * + * You normally should not need to tune this parameter. No connection liveliness + * check is done by default. Value 0 means connections will always be tested for + * validity and negative values mean connections will never be tested. + * + * **Default**: ```undefined``` (Disabled) + * + * @type {number|undefined} + */ + this.connectionLivenessCheckTimeout = undefined // Disabled + /** * Specify socket connection timeout in milliseconds. * diff --git a/packages/neo4j-driver-deno/lib/core/types.ts b/packages/neo4j-driver-deno/lib/core/types.ts index 45a4ab67d..f8cc7a735 100644 --- a/packages/neo4j-driver-deno/lib/core/types.ts +++ b/packages/neo4j-driver-deno/lib/core/types.ts @@ -71,6 +71,7 @@ export class Config { maxTransactionRetryTime?: number maxConnectionLifetime?: number connectionAcquisitionTimeout?: number + connectionLivenessCheckTimeout?: number connectionTimeout?: number disableLosslessIntegers?: boolean notificationFilter?: NotificationFilter @@ -184,6 +185,33 @@ export class Config { */ this.maxTransactionRetryTime = 30000 // 30 seconds + /** + * Specify the maximum time in milliseconds the connection can be idle without needing + * to perform a liveness check on acquire from the pool. + * + * Pooled connections that have been idle in the pool for longer than this + * timeout will be tested before they are used again, to ensure they are still live. + * If this option is set too low, an additional network call will be incurred + * when acquiring a connection, which causes a performance hit. + * + * If this is set high, you may receive sessions that are backed by no longer + * live connections, which will lead to exceptions in your application. + * Assuming the database is running, these exceptions will go away if you retry + * acquiring sessions. + * + * Hence, this parameter tunes a balance between the likelihood of your application + * seeing connection problems, and performance. + * + * You normally should not need to tune this parameter. No connection liveliness + * check is done by default. Value 0 means connections will always be tested for + * validity and negative values mean connections will never be tested. + * + * **Default**: ```undefined``` (Disabled) + * + * @type {number|undefined} + */ + this.connectionLivenessCheckTimeout = undefined // Disabled + /** * Specify socket connection timeout in milliseconds. * From 34e01fdbc6c4b242b9fa36f2d8498f491d4fafe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Thu, 23 Nov 2023 12:38:33 +0100 Subject: [PATCH 02/11] Start work in the liveness check provider --- .../liveness-check-provider.js | 40 ++++++ .../liveness-check-provider.test.js | 136 ++++++++++++++++++ .../liveness-check-provider.js | 40 ++++++ 3 files changed, 216 insertions(+) create mode 100644 packages/bolt-connection/src/connection-provider/liveness-check-provider.js create mode 100644 packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js create mode 100644 packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js diff --git a/packages/bolt-connection/src/connection-provider/liveness-check-provider.js b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js new file mode 100644 index 000000000..2c472ce25 --- /dev/null +++ b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js @@ -0,0 +1,40 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [https://neo4j.com] + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +export default class LivenessCheckProvider { + constructor ({ connectionLivenessCheckTimeout }) { + this._connectionLivenessCheckTimeout = connectionLivenessCheckTimeout + } + + /** + * Checks connection liveness with configured params. + * + * @param {Connection} connection + * @returns {Promise} If liveness checks succeed, throws otherwise + */ + async check (connection) { + if (this._connectionLivenessCheckTimeout == null || + this._connectionLivenessCheckTimeout < 0 || + connection.authToken == null) { + return true + } + + if (this._connectionLivenessCheckTimeout === 0) { + return await connection.resetAndFlush() + .then(() => true) + } + } +} diff --git a/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js b/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js new file mode 100644 index 000000000..052ae03b6 --- /dev/null +++ b/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js @@ -0,0 +1,136 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [https://neo4j.com] + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { newError } from 'neo4j-driver-core' +import LivenessCheckProvider from '../../src/connection-provider/liveness-check-provider' + +describe('LivenessCheckProvider', () => { + describe('.check', () => { + describe.each([ + [undefined, null], + [null, null], + [-1, undefined], + [undefined, undefined], + [null, undefined], + [-1, { scheme: 'none' }], + [undefined, { scheme: 'none' }], + [null, { scheme: 'none' }], + [0, undefined], + [0, null], + [123, undefined], + [3123, null] + ])('when connectionLivenessCheckTimeout=%s and connection.authToken=%s', (connectionLivenessCheckTimeout, authToken) => { + it('should return resolves with true', async () => { + const { provider, connection } = scenario() + + await expect(provider.check(connection)).resolves.toBe(true) + }) + + it('should not reset connection', async () => { + const { provider, connection } = scenario() + + await provider.check(connection) + + expect(connection.resetAndFlush).not.toHaveBeenCalled() + }) + + function scenario () { + const provider = new LivenessCheckProvider({ + connectionLivenessCheckTimeout + }) + + const connection = mockConnection({ authToken }) + + return { provider, connection } + } + }) + + describe.each([ + [0, { scheme: 'none' }, 0], + [0, { scheme: 'none' }, 1234], + [0, { scheme: 'none' }, 3234] + ])('when connectionLivenessCheckTimeout=%s, authToken=%s and connectionIdleFor=%s', (connectionLivenessCheckTimeout, authToken, connectionIdleFor) => { + describe('and resetAndFlush succeed', () => { + it('should return resolves with true', async () => { + const { provider, connection } = scenario() + + await expect(provider.check(connection)).resolves.toBe(true) + }) + + it('should reset connection once', async () => { + const { provider, connection } = scenario() + + await provider.check(connection) + + expect(connection.resetAndFlush).toHaveBeenCalledTimes(1) + }) + + function scenario () { + const provider = new LivenessCheckProvider({ + connectionLivenessCheckTimeout + }) + + const connection = mockConnection({ + authToken, + connectionIdleFor + }) + + return { provider, connection } + } + }) + + describe('and resetAndFlush fail', () => { + const error = newError('Something wrong is not right') + + it('should reject with expected error', async () => { + const { provider, connection } = scenario() + + await expect(provider.check(connection)).rejects.toBe(error) + }) + + it('should reset connection once', async () => { + const { provider, connection } = scenario() + + await provider.check(connection).catch(() => {}) + + expect(connection.resetAndFlush).toHaveBeenCalledTimes(1) + }) + + function scenario () { + const provider = new LivenessCheckProvider({ + connectionLivenessCheckTimeout + }) + + const connection = mockConnection({ + authToken, + connectionIdleFor, + resetAndFlushPromise: Promise.reject(error) + }) + + return { provider, connection } + } + }) + }) + }) + + function mockConnection ({ resetAndFlushPromise, authToken } = {}) { + return { + resetAndFlush: jest.fn(() => resetAndFlushPromise || Promise.resolve()), + authToken + } + } +}) diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js new file mode 100644 index 000000000..2c472ce25 --- /dev/null +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js @@ -0,0 +1,40 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [https://neo4j.com] + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +export default class LivenessCheckProvider { + constructor ({ connectionLivenessCheckTimeout }) { + this._connectionLivenessCheckTimeout = connectionLivenessCheckTimeout + } + + /** + * Checks connection liveness with configured params. + * + * @param {Connection} connection + * @returns {Promise} If liveness checks succeed, throws otherwise + */ + async check (connection) { + if (this._connectionLivenessCheckTimeout == null || + this._connectionLivenessCheckTimeout < 0 || + connection.authToken == null) { + return true + } + + if (this._connectionLivenessCheckTimeout === 0) { + return await connection.resetAndFlush() + .then(() => true) + } + } +} From bf3d31afcc8406535f7a523acc44432804d85347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Thu, 23 Nov 2023 16:40:21 +0100 Subject: [PATCH 03/11] Add idleTimestamp to Connection and implement tests for LivenessCheckProvider --- .../liveness-check-provider.js | 17 +++-- .../src/connection/connection-channel.js | 14 +++- .../src/connection/connection-delegate.js | 12 ++++ .../src/connection/connection.js | 12 ++++ .../connection-provider-direct.test.js | 16 +++++ .../connection-provider-routing.test.js | 16 +++++ .../liveness-check-provider.test.js | 64 ++++++++++++------- .../bolt-connection/test/fake-connection.js | 18 +++++- .../liveness-check-provider.js | 17 +++-- .../connection/connection-channel.js | 14 +++- .../connection/connection-delegate.js | 12 ++++ .../bolt-connection/connection/connection.js | 12 ++++ 12 files changed, 185 insertions(+), 39 deletions(-) diff --git a/packages/bolt-connection/src/connection-provider/liveness-check-provider.js b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js index 2c472ce25..e54d6060e 100644 --- a/packages/bolt-connection/src/connection-provider/liveness-check-provider.js +++ b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js @@ -20,11 +20,11 @@ export default class LivenessCheckProvider { } /** - * Checks connection liveness with configured params. - * - * @param {Connection} connection - * @returns {Promise} If liveness checks succeed, throws otherwise - */ + * Checks connection liveness with configured params. + * + * @param {Connection} connection + * @returns {Promise} If liveness checks succeed, throws otherwise + */ async check (connection) { if (this._connectionLivenessCheckTimeout == null || this._connectionLivenessCheckTimeout < 0 || @@ -32,9 +32,14 @@ export default class LivenessCheckProvider { return true } - if (this._connectionLivenessCheckTimeout === 0) { + const idleFor = Date.now() - connection.idleTimestamp + + if (this._connectionLivenessCheckTimeout === 0 || + idleFor > this._connectionLivenessCheckTimeout) { return await connection.resetAndFlush() .then(() => true) } + + return true } } diff --git a/packages/bolt-connection/src/connection/connection-channel.js b/packages/bolt-connection/src/connection/connection-channel.js index fd3ec73d2..f291b7ec3 100644 --- a/packages/bolt-connection/src/connection/connection-channel.js +++ b/packages/bolt-connection/src/connection/connection-channel.js @@ -130,7 +130,7 @@ export default class ChannelConnection extends Connection { this._id = idGenerator++ this._address = address this._server = { address: address.asHostPort() } - this.creationTimestamp = Date.now() + this._creationTimestamp = Date.now() this._disableLosslessIntegers = disableLosslessIntegers this._ch = channel this._chunker = chunker @@ -220,6 +220,18 @@ export default class ChannelConnection extends Connection { this._dbConnectionId = value } + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + + get creationTimestamp () { + return this._creationTimestamp + } + /** * Send initialization message. * @param {string} userAgent the user agent for this driver. diff --git a/packages/bolt-connection/src/connection/connection-delegate.js b/packages/bolt-connection/src/connection/connection-delegate.js index 14c005cb0..ea24c1210 100644 --- a/packages/bolt-connection/src/connection/connection-delegate.js +++ b/packages/bolt-connection/src/connection/connection-delegate.js @@ -93,6 +93,18 @@ export default class DelegateConnection extends Connection { this._delegate.version = value } + get creationTimestamp () { + return this._delegate.creationTimestamp + } + + set idleTimestamp (value) { + this._delegate.idleTimestamp = value + } + + get idleTimestamp () { + return this._delegate.idleTimestamp + } + isOpen () { return this._delegate.isOpen() } diff --git a/packages/bolt-connection/src/connection/connection.js b/packages/bolt-connection/src/connection/connection.js index 9e8f0bcff..a77619a3b 100644 --- a/packages/bolt-connection/src/connection/connection.js +++ b/packages/bolt-connection/src/connection/connection.js @@ -51,6 +51,18 @@ export default class Connection extends CoreConnection { throw new Error('not implemented') } + get creationTimestamp () { + throw new Error('not implemented') + } + + set idleTimestamp (value) { + throw new Error('not implemented') + } + + get idleTimestamp () { + throw new Error('not implemented') + } + /** * @returns {BoltProtocol} the underlying bolt protocol assigned to this connection */ diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js index 24b9ef935..5fa2dcc16 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js @@ -812,6 +812,22 @@ class FakeConnection extends Connection { return this._supportsReAuth } + set creationTimestamp (value) { + this._creationTimestamp = value + } + + get creationTimestamp () { + return this._creationTimestamp + } + + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + async close () { this._closed = true } diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js index e93835617..07211cf17 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js @@ -3942,6 +3942,22 @@ class FakeConnection extends Connection { this._closed = true } + set creationTimestamp (value) { + this._creationTimestamp = value + } + + get creationTimestamp () { + return this._creationTimestamp + } + + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + isOpen () { return !this._closed } diff --git a/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js b/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js index 052ae03b6..2a3eacfa9 100644 --- a/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js +++ b/packages/bolt-connection/test/connection-provider/liveness-check-provider.test.js @@ -20,20 +20,7 @@ import LivenessCheckProvider from '../../src/connection-provider/liveness-check- describe('LivenessCheckProvider', () => { describe('.check', () => { - describe.each([ - [undefined, null], - [null, null], - [-1, undefined], - [undefined, undefined], - [null, undefined], - [-1, { scheme: 'none' }], - [undefined, { scheme: 'none' }], - [null, { scheme: 'none' }], - [0, undefined], - [0, null], - [123, undefined], - [3123, null] - ])('when connectionLivenessCheckTimeout=%s and connection.authToken=%s', (connectionLivenessCheckTimeout, authToken) => { + describe.each(noNetworkNeededFixture())('when connectionLivenessCheckTimeout=%s, connection.authToken=%s and idleFor=%s', (connectionLivenessCheckTimeout, authToken, idleFor) => { it('should return resolves with true', async () => { const { provider, connection } = scenario() @@ -59,11 +46,7 @@ describe('LivenessCheckProvider', () => { } }) - describe.each([ - [0, { scheme: 'none' }, 0], - [0, { scheme: 'none' }, 1234], - [0, { scheme: 'none' }, 3234] - ])('when connectionLivenessCheckTimeout=%s, authToken=%s and connectionIdleFor=%s', (connectionLivenessCheckTimeout, authToken, connectionIdleFor) => { + describe.each(networkNeededFixture())('when connectionLivenessCheckTimeout=%s, connection.authToken=%s and idleFor=%s', (connectionLivenessCheckTimeout, authToken, idleFor) => { describe('and resetAndFlush succeed', () => { it('should return resolves with true', async () => { const { provider, connection } = scenario() @@ -74,7 +57,7 @@ describe('LivenessCheckProvider', () => { it('should reset connection once', async () => { const { provider, connection } = scenario() - await provider.check(connection) + await provider.check(connection).catch(() => {}) expect(connection.resetAndFlush).toHaveBeenCalledTimes(1) }) @@ -86,7 +69,7 @@ describe('LivenessCheckProvider', () => { const connection = mockConnection({ authToken, - connectionIdleFor + idleFor }) return { provider, connection } @@ -117,7 +100,7 @@ describe('LivenessCheckProvider', () => { const connection = mockConnection({ authToken, - connectionIdleFor, + idleFor, resetAndFlushPromise: Promise.reject(error) }) @@ -127,10 +110,43 @@ describe('LivenessCheckProvider', () => { }) }) - function mockConnection ({ resetAndFlushPromise, authToken } = {}) { + function noNetworkNeededFixture () { + // [connectionLivenessCheckTimeout, authToken, idleFor] + return [ + [undefined, null, 1245], + [null, null, 30000], + [-1, undefined, 30000], + [undefined, undefined, 30000], + [null, undefined, 30000], + [-1, { scheme: 'none' }, 30000], + [undefined, { scheme: 'none' }, 30000], + [null, { scheme: 'none' }, 30000], + [0, undefined, 30000], + [0, null, 30000], + [123, undefined, 30000], + [3123, null, 30000], + [30000, { scheme: 'none' }, 30000], + [29999, { scheme: 'none' }, 30000], + [1, { scheme: 'none' }, 30000] + ] + } + + function networkNeededFixture () { + // [connectionLivenessCheckTimeout, authToken, idleFor] + return [ + [0, { scheme: 'none' }, 0], + [0, { scheme: 'none' }, 1234], + [0, { scheme: 'none' }, 3234], + [1000, { scheme: 'none' }, 3234], + [3233, { scheme: 'none' }, 3234] + ] + } + + function mockConnection ({ resetAndFlushPromise, authToken, idleFor } = {}) { return { resetAndFlush: jest.fn(() => resetAndFlushPromise || Promise.resolve()), - authToken + authToken, + idleTimestamp: idleFor ? Date.now() - idleFor : undefined } } }) diff --git a/packages/bolt-connection/test/fake-connection.js b/packages/bolt-connection/test/fake-connection.js index 18f99e595..80f446d93 100644 --- a/packages/bolt-connection/test/fake-connection.js +++ b/packages/bolt-connection/test/fake-connection.js @@ -32,7 +32,7 @@ export default class FakeConnection extends Connection { this._id = 0 this._databaseId = null this._requestRoutingInformationMock = null - this.creationTimestamp = Date.now() + this._creationTimestamp = Date.now() this.resetInvoked = 0 this.releaseInvoked = 0 @@ -70,6 +70,22 @@ export default class FakeConnection extends Connection { this._server.version = value } + set creationTimestamp (value) { + this._creationTimestamp = value + } + + get creationTimestamp () { + return this._creationTimestamp + } + + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + protocol () { // return fake protocol object that simply records seen queries and parameters return { diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js index 2c472ce25..e54d6060e 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js @@ -20,11 +20,11 @@ export default class LivenessCheckProvider { } /** - * Checks connection liveness with configured params. - * - * @param {Connection} connection - * @returns {Promise} If liveness checks succeed, throws otherwise - */ + * Checks connection liveness with configured params. + * + * @param {Connection} connection + * @returns {Promise} If liveness checks succeed, throws otherwise + */ async check (connection) { if (this._connectionLivenessCheckTimeout == null || this._connectionLivenessCheckTimeout < 0 || @@ -32,9 +32,14 @@ export default class LivenessCheckProvider { return true } - if (this._connectionLivenessCheckTimeout === 0) { + const idleFor = Date.now() - connection.idleTimestamp + + if (this._connectionLivenessCheckTimeout === 0 || + idleFor > this._connectionLivenessCheckTimeout) { return await connection.resetAndFlush() .then(() => true) } + + return true } } diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-channel.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-channel.js index 10de55429..6a4dbe193 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-channel.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-channel.js @@ -130,7 +130,7 @@ export default class ChannelConnection extends Connection { this._id = idGenerator++ this._address = address this._server = { address: address.asHostPort() } - this.creationTimestamp = Date.now() + this._creationTimestamp = Date.now() this._disableLosslessIntegers = disableLosslessIntegers this._ch = channel this._chunker = chunker @@ -220,6 +220,18 @@ export default class ChannelConnection extends Connection { this._dbConnectionId = value } + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + + get creationTimestamp () { + return this._creationTimestamp + } + /** * Send initialization message. * @param {string} userAgent the user agent for this driver. diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-delegate.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-delegate.js index 425ee1ae2..6388f7a6a 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-delegate.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection-delegate.js @@ -93,6 +93,18 @@ export default class DelegateConnection extends Connection { this._delegate.version = value } + get creationTimestamp () { + return this._delegate.creationTimestamp + } + + set idleTimestamp (value) { + this._delegate.idleTimestamp = value + } + + get idleTimestamp () { + return this._delegate.idleTimestamp + } + isOpen () { return this._delegate.isOpen() } diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection.js index 5261be0c4..d23c18188 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection/connection.js @@ -51,6 +51,18 @@ export default class Connection extends CoreConnection { throw new Error('not implemented') } + get creationTimestamp () { + throw new Error('not implemented') + } + + set idleTimestamp (value) { + throw new Error('not implemented') + } + + get idleTimestamp () { + throw new Error('not implemented') + } + /** * @returns {BoltProtocol} the underlying bolt protocol assigned to this connection */ From db2ce2d378a40426888416fd96b620ec9b5e8260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Thu, 23 Nov 2023 16:50:46 +0100 Subject: [PATCH 04/11] Small refactory --- .../connection-provider/liveness-check-provider.js | 12 +++++++++--- .../connection-provider/liveness-check-provider.js | 12 +++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/bolt-connection/src/connection-provider/liveness-check-provider.js b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js index e54d6060e..4d360b635 100644 --- a/packages/bolt-connection/src/connection-provider/liveness-check-provider.js +++ b/packages/bolt-connection/src/connection-provider/liveness-check-provider.js @@ -26,9 +26,7 @@ export default class LivenessCheckProvider { * @returns {Promise} If liveness checks succeed, throws otherwise */ async check (connection) { - if (this._connectionLivenessCheckTimeout == null || - this._connectionLivenessCheckTimeout < 0 || - connection.authToken == null) { + if (this._isCheckDisabled || this._isNewlyCreatedConnection(connection)) { return true } @@ -42,4 +40,12 @@ export default class LivenessCheckProvider { return true } + + get _isCheckDisabled () { + return this._connectionLivenessCheckTimeout == null || this._connectionLivenessCheckTimeout < 0 + } + + _isNewlyCreatedConnection (connection) { + return connection.authToken == null + } } diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js index e54d6060e..4d360b635 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/liveness-check-provider.js @@ -26,9 +26,7 @@ export default class LivenessCheckProvider { * @returns {Promise} If liveness checks succeed, throws otherwise */ async check (connection) { - if (this._connectionLivenessCheckTimeout == null || - this._connectionLivenessCheckTimeout < 0 || - connection.authToken == null) { + if (this._isCheckDisabled || this._isNewlyCreatedConnection(connection)) { return true } @@ -42,4 +40,12 @@ export default class LivenessCheckProvider { return true } + + get _isCheckDisabled () { + return this._connectionLivenessCheckTimeout == null || this._connectionLivenessCheckTimeout < 0 + } + + _isNewlyCreatedConnection (connection) { + return connection.authToken == null + } } From 47eb65ee42533e93a2c6b0d88beec3c46c24dd5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 10:17:10 +0100 Subject: [PATCH 05/11] Add LivenessCheckProvider to PooledConnectionProvider --- .../connection-provider-pooled.js | 12 ++++++++++++ .../connection-provider-pooled.js | 12 ++++++++++++ .../test/internal/fake-connection.js | 18 +++++++++++++++++- 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/packages/bolt-connection/src/connection-provider/connection-provider-pooled.js b/packages/bolt-connection/src/connection-provider/connection-provider-pooled.js index fb32b1ca6..d15de0cd9 100644 --- a/packages/bolt-connection/src/connection-provider/connection-provider-pooled.js +++ b/packages/bolt-connection/src/connection-provider/connection-provider-pooled.js @@ -20,6 +20,7 @@ import Pool, { PoolConfig } from '../pool' import { error, ConnectionProvider, ServerInfo, newError } from 'neo4j-driver-core' import AuthenticationProvider from './authentication-provider' import { object } from '../lang' +import LivenessCheckProvider from './liveness-check-provider' const { SERVICE_UNAVAILABLE } = error const AUTHENTICATION_ERRORS = [ @@ -40,6 +41,7 @@ export default class PooledConnectionProvider extends ConnectionProvider { this._config = config this._log = log this._authenticationProvider = new AuthenticationProvider({ authTokenManager, userAgent, boltAgent }) + this._livenessCheckProvider = new LivenessCheckProvider({ connectionLivenessCheckTimeout: config.connectionLivenessCheckTimeout }) this._userAgent = userAgent this._boltAgent = boltAgent this._createChannelConnection = @@ -81,6 +83,7 @@ export default class PooledConnectionProvider extends ConnectionProvider { _createConnection ({ auth }, address, release) { return this._createChannelConnection(address).then(connection => { connection.release = () => { + connection.idleTimestamp = Date.now() return release(address, connection) } this._openConnections[connection.id] = connection @@ -100,6 +103,15 @@ export default class PooledConnectionProvider extends ConnectionProvider { return false } + try { + await this._livenessCheckProvider.check(conn) + } catch (error) { + this._log.debug( + `The connection ${conn.id} is not alive because of an error ${error.code} '${error.message}'` + ) + return false + } + try { await this._authenticationProvider.authenticate({ connection: conn, auth, skipReAuth }) return true diff --git a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/connection-provider-pooled.js b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/connection-provider-pooled.js index bb1332cbd..0476cc687 100644 --- a/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/connection-provider-pooled.js +++ b/packages/neo4j-driver-deno/lib/bolt-connection/connection-provider/connection-provider-pooled.js @@ -20,6 +20,7 @@ import Pool, { PoolConfig } from '../pool/index.js' import { error, ConnectionProvider, ServerInfo, newError } from '../../core/index.ts' import AuthenticationProvider from './authentication-provider.js' import { object } from '../lang/index.js' +import LivenessCheckProvider from './liveness-check-provider.js' const { SERVICE_UNAVAILABLE } = error const AUTHENTICATION_ERRORS = [ @@ -40,6 +41,7 @@ export default class PooledConnectionProvider extends ConnectionProvider { this._config = config this._log = log this._authenticationProvider = new AuthenticationProvider({ authTokenManager, userAgent, boltAgent }) + this._livenessCheckProvider = new LivenessCheckProvider({ connectionLivenessCheckTimeout: config.connectionLivenessCheckTimeout }) this._userAgent = userAgent this._boltAgent = boltAgent this._createChannelConnection = @@ -81,6 +83,7 @@ export default class PooledConnectionProvider extends ConnectionProvider { _createConnection ({ auth }, address, release) { return this._createChannelConnection(address).then(connection => { connection.release = () => { + connection.idleTimestamp = Date.now() return release(address, connection) } this._openConnections[connection.id] = connection @@ -100,6 +103,15 @@ export default class PooledConnectionProvider extends ConnectionProvider { return false } + try { + await this._livenessCheckProvider.check(conn) + } catch (error) { + this._log.debug( + `The connection ${conn.id} is not alive because of an error ${error.code} '${error.message}'` + ) + return false + } + try { await this._authenticationProvider.authenticate({ connection: conn, auth, skipReAuth }) return true diff --git a/packages/neo4j-driver/test/internal/fake-connection.js b/packages/neo4j-driver/test/internal/fake-connection.js index 11b7c8e47..82089b968 100644 --- a/packages/neo4j-driver/test/internal/fake-connection.js +++ b/packages/neo4j-driver/test/internal/fake-connection.js @@ -38,7 +38,7 @@ export default class FakeConnection extends Connection { this._id = 0 this._databaseId = null this._requestRoutingInformationMock = null - this.creationTimestamp = Date.now() + this._creationTimestamp = Date.now() this.resetInvoked = 0 this.releaseInvoked = 0 @@ -85,6 +85,22 @@ export default class FakeConnection extends Connection { this._authToken = authToken } + set creationTimestamp (value) { + this._creationTimestamp = value + } + + get creationTimestamp () { + return this._creationTimestamp + } + + set idleTimestamp (value) { + this._idleTimestamp = value + } + + get idleTimestamp () { + return this._idleTimestamp + } + protocol () { // return fake protocol object that simply records seen queries and parameters return { From a07c86fffcbf9ef8f9673e70b2818335eee5d13b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 10:46:00 +0100 Subject: [PATCH 06/11] Add testkit backend bits --- packages/testkit-backend/src/feature/common.js | 1 + packages/testkit-backend/src/request-handlers.js | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/packages/testkit-backend/src/feature/common.js b/packages/testkit-backend/src/feature/common.js index 67887a714..69d881f76 100644 --- a/packages/testkit-backend/src/feature/common.js +++ b/packages/testkit-backend/src/feature/common.js @@ -32,6 +32,7 @@ const features = [ 'Feature:API:Driver.VerifyAuthentication', 'Feature:API:Driver.VerifyConnectivity', 'Feature:API:Session:NotificationsConfig', + 'Feature:API:Liveness.Check', 'Optimization:AuthPipelining', 'Optimization:EagerTransactionBegin', 'Optimization:ExecuteQueryPipelining', diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 19bb4ed23..b1f17dd55 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -82,6 +82,10 @@ export function NewDriver ({ neo4j }, context, data, wire) { config.telemetryDisabled = data.telemetryDisabled } + if ('livenessCheckTimeoutMs' in data) { + config.connectionLivenessCheckTimeout = data.livenessCheckTimeoutMs + } + let driver try { driver = neo4j.driver(uri, parsedAuthToken, config) From c17cba65af7acab45670c74dec54ef4daad590fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 11:22:54 +0100 Subject: [PATCH 07/11] Add tests for validateOnAcquire --- .../connection-provider-direct.test.js | 46 +++++++++++++++---- .../connection-provider-routing.test.js | 44 +++++++++++++++--- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js index 5fa2dcc16..eeb2ec295 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js @@ -21,6 +21,7 @@ import { Connection, DelegateConnection } from '../../src/connection' import { authTokenManagers, internal, newError, ServerInfo, staticAuthTokenManager } from 'neo4j-driver-core' import AuthenticationProvider from '../../src/connection-provider/authentication-provider' import { functional } from '../../src/lang' +import LivenessCheckProvider from '../../src/connection-provider/liveness-check-provider' const { serverAddress: { ServerAddress }, @@ -361,26 +362,27 @@ describe('constructor', () => { const connection = new FakeConnection(server0) connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({ auth }, connection)).resolves.toBe(true) expect(authenticationProviderHook.authenticate).toHaveBeenCalledWith({ connection, auth }) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) }) it.each([ null, undefined, { scheme: 'bearer', credentials: 'token01' } - ])('should return true when connection is open and within the lifetime and authentication fails (auth=%o)', async (auth) => { + ])('should return false when connection is open and within the lifetime and authentication fails (auth=%o)', async (auth) => { const connection = new FakeConnection(server0) const error = newError('failed') const authenticationProvider = jest.fn(() => Promise.reject(error)) connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook, log } = setup({ authenticationProvider }) + const { validateOnAcquire, authenticationProviderHook, log, livenessCheckProviderHook } = setup({ authenticationProvider }) await expect(validateOnAcquire({ auth }, connection)).resolves.toBe(false) @@ -391,6 +393,7 @@ describe('constructor', () => { expect(log.debug).toHaveBeenCalledWith( `The connection ${connection.id} is not valid because of an error ${error.code} '${error.message}'` ) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) }) it.each([ @@ -401,13 +404,32 @@ describe('constructor', () => { const auth = {} connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({ auth, skipReAuth }, connection)).resolves.toBe(true) expect(authenticationProviderHook.authenticate).toHaveBeenCalledWith({ connection, auth, skipReAuth }) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) + }) + + it('should return false when liveness checks fails', async () => { + const connection = new FakeConnection(server0) + connection.creationTimestamp = Date.now() + const error = newError('#themessage', '#thecode') + + const { validateOnAcquire, authenticationProviderHook, log, livenessCheckProviderHook } = setup({ + livenessCheckProvider: () => Promise.reject(error) + }) + + await expect(validateOnAcquire({}, connection)).resolves.toBe(false) + + expect(livenessCheckProviderHook.check).toBeCalledWith(connection) + expect(log.debug).toBeCalledWith( + `The connection ${connection.id} is not alive because of an error ${error.code} '${error.message}'` + ) + expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() }) it('should return false when connection is closed and within the lifetime', async () => { @@ -415,20 +437,22 @@ describe('constructor', () => { connection.creationTimestamp = Date.now() await connection.close() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) it('should return false when connection is open and out of the lifetime', async () => { const connection = new FakeConnection(server0) connection.creationTimestamp = Date.now() - 4000 - const { validateOnAcquire, authenticationProviderHook } = setup({ maxConnectionLifetime: 3000 }) + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup({ maxConnectionLifetime: 3000 }) await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) it('should return false when connection is closed and out of the lifetime', async () => { @@ -436,10 +460,11 @@ describe('constructor', () => { await connection.close() connection.creationTimestamp = Date.now() - 4000 - const { validateOnAcquire, authenticationProviderHook } = setup({ maxConnectionLifetime: 3000 }) + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup({ maxConnectionLifetime: 3000 }) await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) }) @@ -492,14 +517,17 @@ describe('constructor', () => { }) }) - function setup ({ createConnection, authenticationProvider, maxConnectionLifetime } = {}) { + function setup ({ createConnection, authenticationProvider, maxConnectionLifetime, livenessCheckProvider } = {}) { const newPool = jest.fn((...args) => new Pool(...args)) const log = new Logger('debug', () => undefined) jest.spyOn(log, 'debug') const createChannelConnectionHook = createConnection || jest.fn(async (address) => new FakeConnection(address)) const authenticationProviderHook = new AuthenticationProvider({ }) + const livenessCheckProviderHook = new LivenessCheckProvider({}) jest.spyOn(authenticationProviderHook, 'authenticate') .mockImplementation(authenticationProvider || jest.fn(({ connection }) => Promise.resolve(connection))) + jest.spyOn(livenessCheckProviderHook, 'check') + .mockImplementation(livenessCheckProvider || jest.fn(() => Promise.resolve(true))) const provider = new DirectConnectionProvider({ newPool, config: { @@ -510,11 +538,13 @@ describe('constructor', () => { }) provider._createChannelConnection = createChannelConnectionHook provider._authenticationProvider = authenticationProviderHook + provider._livenessCheckProvider = livenessCheckProviderHook return { provider, ...newPool.mock.calls[0][0], createChannelConnectionHook, authenticationProviderHook, + livenessCheckProviderHook, log } } diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js index 07211cf17..f809c4900 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js @@ -33,6 +33,7 @@ import RoutingConnectionProvider from '../../src/connection-provider/connection- import { DelegateConnection, Connection } from '../../src/connection' import AuthenticationProvider from '../../src/connection-provider/authentication-provider' import { functional } from '../../src/lang' +import LivenessCheckProvider from '../../src/connection-provider/liveness-check-provider' const { serverAddress: { ServerAddress }, @@ -3265,13 +3266,14 @@ describe.each([ const connection = new FakeConnection(server0) connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({ auth }, connection)).resolves.toBe(true) expect(authenticationProviderHook.authenticate).toHaveBeenCalledWith({ connection, auth }) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) }) it.each([ @@ -3284,7 +3286,7 @@ describe.each([ const authenticationProvider = jest.fn(() => Promise.reject(error)) connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook, log } = setup({ authenticationProvider }) + const { validateOnAcquire, authenticationProviderHook, log, livenessCheckProviderHook } = setup({ authenticationProvider }) await expect(validateOnAcquire({ auth }, connection)).resolves.toBe(false) @@ -3295,6 +3297,7 @@ describe.each([ expect(log.debug).toHaveBeenCalledWith( `The connection ${connection.id} is not valid because of an error ${error.code} '${error.message}'` ) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) }) it.each([ @@ -3305,13 +3308,32 @@ describe.each([ const auth = {} connection.creationTimestamp = Date.now() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({ auth, skipReAuth }, connection)).resolves.toBe(true) expect(authenticationProviderHook.authenticate).toHaveBeenCalledWith({ connection, auth, skipReAuth }) + expect(livenessCheckProviderHook.check).toHaveBeenCalledWith(connection) + }) + + it('should return false when liveness checks fails', async () => { + const connection = new FakeConnection(server0) + connection.creationTimestamp = Date.now() + const error = newError('#themessage', '#thecode') + + const { validateOnAcquire, authenticationProviderHook, log, livenessCheckProviderHook } = setup({ + livenessCheckProvider: () => Promise.reject(error) + }) + + await expect(validateOnAcquire({}, connection)).resolves.toBe(false) + + expect(livenessCheckProviderHook.check).toBeCalledWith(connection) + expect(log.debug).toBeCalledWith( + `The connection ${connection.id} is not alive because of an error ${error.code} '${error.message}'` + ) + expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() }) it('should return false when connection is closed and within the lifetime', async () => { @@ -3319,20 +3341,22 @@ describe.each([ connection.creationTimestamp = Date.now() await connection.close() - const { validateOnAcquire, authenticationProviderHook } = setup() + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup() await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) it('should return false when connection is open and out of the lifetime', async () => { const connection = new FakeConnection(server0) connection.creationTimestamp = Date.now() - 4000 - const { validateOnAcquire, authenticationProviderHook } = setup({ maxConnectionLifetime: 3000 }) + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup({ maxConnectionLifetime: 3000 }) await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) it('should return false when connection is closed and out of the lifetime', async () => { @@ -3340,10 +3364,11 @@ describe.each([ await connection.close() connection.creationTimestamp = Date.now() - 4000 - const { validateOnAcquire, authenticationProviderHook } = setup({ maxConnectionLifetime: 3000 }) + const { validateOnAcquire, authenticationProviderHook, livenessCheckProviderHook } = setup({ maxConnectionLifetime: 3000 }) await expect(validateOnAcquire({}, connection)).resolves.toBe(false) expect(authenticationProviderHook.authenticate).not.toHaveBeenCalled() + expect(livenessCheckProviderHook.check).not.toHaveBeenCalled() }) }) @@ -3396,14 +3421,17 @@ describe.each([ }) }) - function setup ({ createConnection, authenticationProvider, maxConnectionLifetime } = {}) { + function setup ({ createConnection, authenticationProvider, maxConnectionLifetime, livenessCheckProvider } = {}) { const newPool = jest.fn((...args) => new Pool(...args)) const log = new Logger('debug', () => undefined) jest.spyOn(log, 'debug') const createChannelConnectionHook = createConnection || jest.fn(async (address) => new FakeConnection(address)) const authenticationProviderHook = new AuthenticationProvider({}) + const livenessCheckProviderHook = new LivenessCheckProvider({}) jest.spyOn(authenticationProviderHook, 'authenticate') .mockImplementation(authenticationProvider || jest.fn(({ connection }) => Promise.resolve(connection))) + jest.spyOn(livenessCheckProviderHook, 'check') + .mockImplementation(livenessCheckProvider || jest.fn(() => Promise.resolve(true))) const provider = new RoutingConnectionProvider({ newPool, config: { @@ -3414,11 +3442,13 @@ describe.each([ }) provider._createChannelConnection = createChannelConnectionHook provider._authenticationProvider = authenticationProviderHook + provider._livenessCheckProvider = livenessCheckProviderHook return { provider, ...newPool.mock.calls[0][0], createChannelConnectionHook, authenticationProviderHook, + livenessCheckProviderHook, log } } From 54d5d9cf81ff4d981a89edb3dc2c043a433cbc1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 12:39:22 +0100 Subject: [PATCH 08/11] Update release tests for checking the idleTimestamp --- .../connection-provider-direct.test.js | 21 ++++++++++++------- .../connection-provider-routing.test.js | 21 ++++++++++++------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js index eeb2ec295..ff2f10ee0 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-direct.test.js @@ -282,16 +282,23 @@ describe('constructor', () => { }) it('should register the release function into the connection', async () => { - const { create } = setup() - const releaseResult = { property: 'some property' } - const release = jest.fn(() => releaseResult) + jest.useFakeTimers() + try { + const { create } = setup() + const releaseResult = { property: 'some property' } + const release = jest.fn(() => releaseResult) - const connection = await create({}, server0, release) + const connection = await create({}, server0, release) + connection.idleTimestamp = -1234 - const released = connection.release() + const released = connection.release() - expect(released).toBe(releaseResult) - expect(release).toHaveBeenCalledWith(server0, connection) + expect(released).toBe(releaseResult) + expect(release).toHaveBeenCalledWith(server0, connection) + expect(connection.idleTimestamp).toBeCloseTo(Date.now()) + } finally { + jest.useRealTimers() + } }) it.each([ diff --git a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js index f809c4900..c1eeeb794 100644 --- a/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js +++ b/packages/bolt-connection/test/connection-provider/connection-provider-routing.test.js @@ -3186,16 +3186,23 @@ describe.each([ }) it('should register the release function into the connection', async () => { - const { create } = setup() - const releaseResult = { property: 'some property' } - const release = jest.fn(() => releaseResult) + jest.useFakeTimers() + try { + const { create } = setup() + const releaseResult = { property: 'some property' } + const release = jest.fn(() => releaseResult) - const connection = await create({}, server0, release) + const connection = await create({}, server0, release) + connection.idleTimestamp = -1234 - const released = connection.release() + const released = connection.release() - expect(released).toBe(releaseResult) - expect(release).toHaveBeenCalledWith(server0, connection) + expect(released).toBe(releaseResult) + expect(release).toHaveBeenCalledWith(server0, connection) + expect(connection.idleTimestamp).toBeCloseTo(Date.now()) + } finally { + jest.useRealTimers() + } }) it.each([ From 718b844575994976b394bac3fa20a9bee049ef29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 13:17:42 +0100 Subject: [PATCH 09/11] Validate connection liveness check timeout --- packages/core/src/driver.ts | 22 +++++++ packages/core/test/driver.test.ts | 60 ++++++++++++++++++- packages/neo4j-driver-deno/lib/core/driver.ts | 24 +++++++- 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 217188d34..96d1fdeec 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -107,6 +107,7 @@ interface DriverConfig { fetchSize?: number logging?: LoggingConfig notificationFilter?: NotificationFilter + connectionLivenessCheckTimeout?: number } /** @@ -907,6 +908,9 @@ function sanitizeConfig (config: any): void { DEFAULT_FETCH_SIZE ) config.connectionTimeout = extractConnectionTimeout(config) + + config.connectionLivenessCheckTimeout = + validateConnectionLivenessCheckTimeoutSizeValue(config.connectionLivenessCheckTimeout) } /** @@ -962,6 +966,24 @@ function extractConnectionTimeout (config: any): number | null { } } +/** + * @private + */ +function validateConnectionLivenessCheckTimeoutSizeValue ( + rawValue: any +): number | undefined { + if (rawValue == null) { + return undefined + } + const connectionLivenessCheckTimeout = parseInt(rawValue, 10) + if (connectionLivenessCheckTimeout < 0 || Number.isNaN(connectionLivenessCheckTimeout)) { + throw new Error( + `The connectionLivenessCheckTimeout can only be a positive value or 0 for always. However connectionLivenessCheckTimeout = ${connectionLivenessCheckTimeout}` + ) + } + return connectionLivenessCheckTimeout +} + /** * @private * @returns {ConfiguredCustomResolver} new custom resolver that wraps the passed-in resolver function. diff --git a/packages/core/test/driver.test.ts b/packages/core/test/driver.test.ts index fd5c9a16d..48b872703 100644 --- a/packages/core/test/driver.test.ts +++ b/packages/core/test/driver.test.ts @@ -15,7 +15,7 @@ * limitations under the License. */ /* eslint-disable @typescript-eslint/promise-function-async */ -import { bookmarkManager, ConnectionProvider, EagerResult, newError, NotificationFilter, Result, ResultSummary, ServerInfo, Session, TransactionConfig } from '../src' +import { bookmarkManager, ConnectionProvider, EagerResult, int, newError, NotificationFilter, Result, ResultSummary, ServerInfo, Session, TransactionConfig } from '../src' import Driver, { QueryConfig, READ, routing } from '../src/driver' import { Bookmarks } from '../src/internal/bookmarks' import { Logger } from '../src/internal/logger' @@ -603,6 +603,64 @@ describe('Driver', () => { await driver.close() }) }) + + describe('config', () => { + describe('connectionLivenessCheckTimeout', () => { + it.each([ + [undefined, undefined], + [null, undefined], + [0, 0], + [BigInt(0), 0], + [int(0), 0], + ['0', 0], + [3000, 3000], + [BigInt(3000), 3000], + [int(3000), 3000], + ['3000', 3000] + ])('should sanitize value % to %s', async (configured, expected) => { + const createConnectionProviderMock = jest.fn(mockCreateConnectonProvider(connectionProvider)) + const driver = new Driver( + META_INFO, + // @ts-expect-error + { connectionLivenessCheckTimeout: configured }, + createConnectionProviderMock, + createSession + ) + + driver._getOrCreateConnectionProvider() + + expect(createConnectionProviderMock).toHaveBeenCalledWith( + expect.any(Number), + expect.objectContaining({ connectionLivenessCheckTimeout: expected }), + expect.any(Object), + expect.any(Object) + ) + + await driver.close() + }) + + it.each([ + [-1, -1], + [int(-1), -1], + [BigInt(-1), -1], + ['-1', -1], + [{}, NaN], + ['a', NaN] + ])('should fail in invalid values', async (configured, expected) => { + const createConnectionProviderMock = jest.fn(mockCreateConnectonProvider(connectionProvider)) + + expect(() => new Driver( + META_INFO, + // @ts-expect-error + { connectionLivenessCheckTimeout: configured }, + createConnectionProviderMock, + createSession + )).toThrow(new Error(`The connectionLivenessCheckTimeout can only be a positive value or 0 for always. However connectionLivenessCheckTimeout = ${expected}`)) + + expect(createConnectionProviderMock).not.toHaveBeenCalled() + }) + }) + }) }) function mockCreateConnectonProvider (connectionProvider: ConnectionProvider) { diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index df2fe34dc..66156d548 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -106,7 +106,8 @@ interface DriverConfig { trust?: TrustStrategy fetchSize?: number logging?: LoggingConfig - notificationFilter?: NotificationFilter + notificationFilter?: NotificationFilter, + connectionLivenessCheckTimeout?: number } /** @@ -907,6 +908,9 @@ function sanitizeConfig (config: any): void { DEFAULT_FETCH_SIZE ) config.connectionTimeout = extractConnectionTimeout(config) + + config.connectionLivenessCheckTimeout = + validateConnectionLivenessCheckTimeoutSizeValue(config.connectionLivenessCheckTimeout) } /** @@ -962,6 +966,24 @@ function extractConnectionTimeout (config: any): number | null { } } +/** + * @private + */ +function validateConnectionLivenessCheckTimeoutSizeValue ( + rawValue: any +): number | undefined { + if (rawValue == null) { + return undefined + } + const connectionLivenessCheckTimeout = parseInt(rawValue, 10) + if (connectionLivenessCheckTimeout < 0 || Number.isNaN(connectionLivenessCheckTimeout)) { + throw new Error( + `The connectionLivenessCheckTimeout can only be a positive value or 0 for always. However connectionLivenessCheckTimeout = ${connectionLivenessCheckTimeout}` + ) + } + return connectionLivenessCheckTimeout +} + /** * @private * @returns {ConfiguredCustomResolver} new custom resolver that wraps the passed-in resolver function. From b704559540aca2a58895b58b2be68d92fed99e07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 24 Nov 2023 13:18:08 +0100 Subject: [PATCH 10/11] Validate connection liveness check timeout --- packages/neo4j-driver-deno/lib/core/driver.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index 66156d548..a6e61c49b 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -106,7 +106,7 @@ interface DriverConfig { trust?: TrustStrategy fetchSize?: number logging?: LoggingConfig - notificationFilter?: NotificationFilter, + notificationFilter?: NotificationFilter connectionLivenessCheckTimeout?: number } @@ -909,7 +909,7 @@ function sanitizeConfig (config: any): void { ) config.connectionTimeout = extractConnectionTimeout(config) - config.connectionLivenessCheckTimeout = + config.connectionLivenessCheckTimeout = validateConnectionLivenessCheckTimeoutSizeValue(config.connectionLivenessCheckTimeout) } @@ -980,7 +980,7 @@ function validateConnectionLivenessCheckTimeoutSizeValue ( throw new Error( `The connectionLivenessCheckTimeout can only be a positive value or 0 for always. However connectionLivenessCheckTimeout = ${connectionLivenessCheckTimeout}` ) - } + } return connectionLivenessCheckTimeout } From 745cb3c029db33e80790e1583033f4cdce27f4f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 27 Nov 2023 17:57:37 +0100 Subject: [PATCH 11/11] Skip tests which needs unification --- packages/testkit-backend/src/skipped-tests/common.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/testkit-backend/src/skipped-tests/common.js b/packages/testkit-backend/src/skipped-tests/common.js index ee7e6e3bd..4d5eb9669 100644 --- a/packages/testkit-backend/src/skipped-tests/common.js +++ b/packages/testkit-backend/src/skipped-tests/common.js @@ -187,6 +187,12 @@ const skippedTests = [ endsWith('test_error_on_rollback_using_tx_run') ) ) + ), + skip( + 'Behaviour pending unification', + ifStartsWith('stub.routing.test_routing_') + .and( + ifEndsWith('.test_should_drop_connections_failing_liveness_check')) ) ]