From c5fd1a9fe3770caef95505c8eef3f125d517991a Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 14 Sep 2017 23:44:39 +0200 Subject: [PATCH 1/2] Connection timeout Added ability to enforce max connection timeout. It is applied to new connections and makes them terminate connection attempt after configured number of milliseconds. Feature is needed because connect part might take minutes in some environments. NodeJS channel uses `net.Socket` when non-encrypted and it's subclass `tls.TLSSocket` when encrypted. Connection timeout is implemented using `socket.setTimeout()` function which triggers a callback after configured millis of inactivity. We first initiate a connection attempt and them set the timeout. If connection establishes before the timeout then later is canceled. Otherwise `timeout` event is fired and socket is destroyed. WebSocket channel used in browser does not have built-in timeout functionality. That is why we simply use `setTimeout()` that closes WebSocket if it does not connect in time. Successful connection cancels the timeout. Feature is turned of by default and can be configured using `connectionTimeout` property in the config. Example: ``` const driver = neo4j.driver( 'bolt://localhost', neo4j.auth.basic('neo4j', 'pwd'), {connectionTimeout: 5000 /* 5 seconds */ }); ``` --- src/v1/index.js | 7 ++++- src/v1/internal/ch-config.js | 55 ++++++++++++++++++++------------- src/v1/internal/ch-node.js | 26 ++++++++++++++++ src/v1/internal/ch-websocket.js | 46 +++++++++++++++++++++++---- test/internal/connector.test.js | 33 ++++++++++++++++++++ test/v1/session.test.js | 29 +++++++++++++++++ test/v1/transaction.test.js | 38 ++++++++++++++++++++++- 7 files changed, 204 insertions(+), 30 deletions(-) diff --git a/src/v1/index.js b/src/v1/index.js index 948259443..4fd9a6977 100644 --- a/src/v1/index.js +++ b/src/v1/index.js @@ -116,7 +116,12 @@ const USER_AGENT = "neo4j-javascript/" + VERSION; * // {@link Session#readTransaction()} and {@link Session#writeTransaction()} functions. These functions * // will retry the given unit of work on `ServiceUnavailable`, `SessionExpired` and transient errors with * // exponential backoff using initial delay of 1 second. Default value is 30000 which is 30 seconds. - * maxTransactionRetryTime: 30000, + * maxTransactionRetryTime: 30000, // 30 seconds + * + * // Specify socket connection timeout in milliseconds. Non-numeric, negative and zero values are treated as an + * // infinite timeout. Connection will be then bound by the timeout configured on the operating system level. + * // Timeout value should be numeric and greater or equal to zero. + * connectionTimeout: 5000, // 5 seconds * } * * @param {string} url The URL for the Neo4j database, for instance "bolt://localhost" diff --git a/src/v1/internal/ch-config.js b/src/v1/internal/ch-config.js index faf6ba10a..80e07b6e8 100644 --- a/src/v1/internal/ch-config.js +++ b/src/v1/internal/ch-config.js @@ -20,38 +20,49 @@ import hasFeature from './features'; import {SERVICE_UNAVAILABLE} from '../error'; +const DEFAULT_CONNECTION_TIMEOUT_MILLIS = 0; // turned off by default + export default class ChannelConfig { constructor(host, port, driverConfig, connectionErrorCode) { this.host = host; this.port = port; - this.encrypted = ChannelConfig._extractEncrypted(driverConfig); - this.trust = ChannelConfig._extractTrust(driverConfig); - this.trustedCertificates = ChannelConfig._extractTrustedCertificates(driverConfig); - this.knownHostsPath = ChannelConfig._extractKnownHostsPath(driverConfig); + this.encrypted = extractEncrypted(driverConfig); + this.trust = extractTrust(driverConfig); + this.trustedCertificates = extractTrustedCertificates(driverConfig); + this.knownHostsPath = extractKnownHostsPath(driverConfig); this.connectionErrorCode = connectionErrorCode || SERVICE_UNAVAILABLE; + this.connectionTimeout = extractConnectionTimeout(driverConfig); } +} - static _extractEncrypted(driverConfig) { - // check if encryption was configured by the user, use explicit null check because we permit boolean value - const encryptionConfigured = driverConfig.encrypted == null; - // default to using encryption if trust-all-certificates is available - return encryptionConfigured ? hasFeature('trust_all_certificates') : driverConfig.encrypted; - } +function extractEncrypted(driverConfig) { + // check if encryption was configured by the user, use explicit null check because we permit boolean value + const encryptionConfigured = driverConfig.encrypted == null; + // default to using encryption if trust-all-certificates is available + return encryptionConfigured ? hasFeature('trust_all_certificates') : driverConfig.encrypted; +} - static _extractTrust(driverConfig) { - if (driverConfig.trust) { - return driverConfig.trust; - } - // default to using TRUST_ALL_CERTIFICATES if it is available - return hasFeature('trust_all_certificates') ? 'TRUST_ALL_CERTIFICATES' : 'TRUST_CUSTOM_CA_SIGNED_CERTIFICATES'; +function extractTrust(driverConfig) { + if (driverConfig.trust) { + return driverConfig.trust; } + // default to using TRUST_ALL_CERTIFICATES if it is available + return hasFeature('trust_all_certificates') ? 'TRUST_ALL_CERTIFICATES' : 'TRUST_CUSTOM_CA_SIGNED_CERTIFICATES'; +} - static _extractTrustedCertificates(driverConfig) { - return driverConfig.trustedCertificates || []; - } +function extractTrustedCertificates(driverConfig) { + return driverConfig.trustedCertificates || []; +} + +function extractKnownHostsPath(driverConfig) { + return driverConfig.knownHosts || null; +} - static _extractKnownHostsPath(driverConfig) { - return driverConfig.knownHosts || null; +function extractConnectionTimeout(driverConfig) { + const configuredTimeout = parseInt(driverConfig.connectionTimeout, 10); + if (!configuredTimeout || configuredTimeout < 0) { + return DEFAULT_CONNECTION_TIMEOUT_MILLIS; } -}; + return configuredTimeout; +} diff --git a/src/v1/internal/ch-node.js b/src/v1/internal/ch-node.js index 4bfa06122..7d4fb3458 100644 --- a/src/v1/internal/ch-node.js +++ b/src/v1/internal/ch-node.js @@ -320,6 +320,8 @@ class NodeChannel { self.write( pending[i] ); } }, this._handleConnectionError); + + this._setupConnectionTimeout(config, this._conn); } _handleConnectionError( err ) { @@ -337,6 +339,30 @@ class NodeChannel { } } + /** + * Setup connection timeout on the socket, if configured. + * @param {ChannelConfig} config - configuration of this channel. + * @param {object} socket - `net.Socket` or `tls.TLSSocket` object. + * @private + */ + _setupConnectionTimeout(config, socket) { + const timeout = config.connectionTimeout; + if (timeout) { + socket.on('connect', () => { + // connected - clear connection timeout + socket.setTimeout(0); + }); + + socket.on('timeout', () => { + // timeout fired - not connected within configured time. cancel timeout and destroy socket + socket.setTimeout(0); + socket.destroy(newError(`Failed to establish connection in ${timeout}ms`, config.connectionErrorCode)); + }); + + socket.setTimeout(timeout); + } + } + isEncrypted() { return this._encrypted; } diff --git a/src/v1/internal/ch-websocket.js b/src/v1/internal/ch-websocket.js index c937fded7..b8437f68e 100644 --- a/src/v1/internal/ch-websocket.js +++ b/src/v1/internal/ch-websocket.js @@ -37,9 +37,7 @@ class WebSocketChannel { this._pending = []; this._error = null; this._handleConnectionError = this._handleConnectionError.bind(this); - this._connectionErrorCode = config.connectionErrorCode; - - this._encrypted = config.encrypted; + this._config = config; let scheme = "ws"; //Allow boolean for backwards compatibility @@ -67,6 +65,9 @@ class WebSocketChannel { } }; this._ws.onopen = function() { + // Connected! Cancel connection timeout + clearTimeout(self._connectionTimeoutId); + // Drain all pending messages let pending = self._pending; self._pending = null; @@ -76,15 +77,28 @@ class WebSocketChannel { }; this._ws.onmessage = (event) => { if( self.onmessage ) { - var b = new HeapBuffer( event.data ); + const b = new HeapBuffer(event.data); self.onmessage( b ); } }; this._ws.onerror = this._handleConnectionError; + + this._connectionTimeoutFired = false; + this._connectionTimeoutId = this._setupConnectionTimeout(config); } _handleConnectionError() { + if (this._connectionTimeoutFired) { + // timeout fired - not connected within configured time + this._error = newError(`Failed to establish connection in ${this._config.connectionTimeout}ms`, this._config.connectionErrorCode); + + if (this.onerror) { + this.onerror(this._error); + } + return; + } + // onerror triggers on websocket close as well.. don't get me started. if( this._open ) { // http://stackoverflow.com/questions/25779831/how-to-catch-websocket-connection-to-ws-xxxnn-failed-connection-closed-be @@ -94,7 +108,7 @@ class WebSocketChannel { "the root cause of the failure. Common reasons include the database being " + "unavailable, using the wrong connection URL or temporary network problems. " + "If you have enabled encryption, ensure your browser is configured to trust the " + - "certificate Neo4j is configured to use. WebSocket `readyState` is: " + this._ws.readyState, this._connectionErrorCode ); + 'certificate Neo4j is configured to use. WebSocket `readyState` is: ' + this._ws.readyState, this._config.connectionErrorCode); if (this.onerror) { this.onerror(this._error); } @@ -102,7 +116,7 @@ class WebSocketChannel { } isEncrypted() { - return this._encrypted; + return this._config.encrypted; } /** @@ -130,6 +144,26 @@ class WebSocketChannel { this._ws.close(); this._ws.onclose = cb; } + + /** + * Set connection timeout on the given WebSocket, if configured. + * @return {number} the timeout id or null. + * @private + */ + _setupConnectionTimeout() { + const timeout = this._config.connectionTimeout; + if (timeout) { + const webSocket = this._ws; + + return setTimeout(() => { + if (webSocket.readyState !== WebSocket.OPEN) { + this._connectionTimeoutFired = true; + webSocket.close(); + } + }, timeout); + } + return null; + } } let available = typeof WebSocket !== 'undefined'; diff --git a/test/internal/connector.test.js b/test/internal/connector.test.js index ba4889a9f..10e384053 100644 --- a/test/internal/connector.test.js +++ b/test/internal/connector.test.js @@ -209,6 +209,14 @@ describe('connector', () => { }); }); + it('should respect connection timeout', done => { + testConnectionTimeout(false, done); + }); + + it('should respect encrypted connection timeout', done => { + testConnectionTimeout(true, done); + }); + function packedHandshakeMessage() { const result = alloc(4); result.putInt32(0, 1); @@ -244,4 +252,29 @@ describe('connector', () => { }; } + function testConnectionTimeout(encrypted, done) { + const boltUri = 'bolt://10.0.0.0'; // use non-routable IP address which never responds + connection = connect(boltUri, {encrypted: encrypted, connectionTimeout: 1000}, 'TestErrorCode'); + + connection.initialize('mydriver/0.0.0', basicAuthToken(), { + onNext: record => { + done.fail('Should not receive records: ' + record); + }, + onCompleted: () => { + done.fail('Should not be able to INIT'); + }, + onError: error => { + expect(error.code).toEqual('TestErrorCode'); + + // in some environments non-routable address results in immediate 'connection refused' error and connect + // timeout is not fired; skip message assertion for such cases, it is important for connect attempt to not hang + if (error.message.indexOf('Failed to establish connection') === 0) { + expect(error.message).toEqual('Failed to establish connection in 1000ms'); + } + + done(); + } + }); + } + }); diff --git a/test/v1/session.test.js b/test/v1/session.test.js index daf6a0dc2..b6dc104ad 100644 --- a/test/v1/session.test.js +++ b/test/v1/session.test.js @@ -965,6 +965,14 @@ describe('session', () => { }); }); + it('should respect connection timeout', done => { + testConnectionTimeout(false, done); + }); + + it('should respect encrypted connection timeout', done => { + testConnectionTimeout(true, done); + }); + function serverIs31OrLater(done) { // lazy way of checking the version number // if server has been set we know it is at least 3.1 @@ -1043,4 +1051,25 @@ describe('session', () => { }); } + function testConnectionTimeout(encrypted, done) { + const boltUri = 'bolt://10.0.0.0'; // use non-routable IP address which never responds + const config = {encrypted: encrypted, connectionTimeout: 1000}; + + const localDriver = neo4j.driver(boltUri, sharedNeo4j.authToken, config); + const session = localDriver.session(); + session.run('RETURN 1').then(() => { + done.fail('Query did not fail'); + }).catch(error => { + expect(error.code).toEqual(neo4j.error.SERVICE_UNAVAILABLE); + + // in some environments non-routable address results in immediate 'connection refused' error and connect + // timeout is not fired; skip message assertion for such cases, it is important for connect attempt to not hang + if (error.message.indexOf('Failed to establish connection') === 0) { + expect(error.message).toEqual('Failed to establish connection in 1000ms'); + } + + done(); + }); + } + }); diff --git a/test/v1/transaction.test.js b/test/v1/transaction.test.js index baa5309b2..8dfe78a39 100644 --- a/test/v1/transaction.test.js +++ b/test/v1/transaction.test.js @@ -29,7 +29,7 @@ describe('transaction', () => { beforeEach(done => { // make jasmine timeout high enough to test unreachable bookmarks originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; - jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 40000; driver = neo4j.driver("bolt://localhost", sharedNeo4j.authToken); driver.onCompleted = meta => { @@ -500,6 +500,14 @@ describe('transaction', () => { }); }); + it('should respect socket connection timeout', done => { + testConnectionTimeout(false, done); + }); + + it('should respect TLS socket connection timeout', done => { + testConnectionTimeout(true, done); + }); + function expectSyntaxError(error) { expect(error.code).toBe('Neo.ClientError.Statement.SyntaxError'); } @@ -519,4 +527,32 @@ describe('transaction', () => { } return false; } + + function testConnectionTimeout(encrypted, done) { + const boltUri = 'bolt://10.0.0.0'; // use non-routable IP address which never responds + const config = {encrypted: encrypted, connectionTimeout: 1000}; + + const localDriver = neo4j.driver(boltUri, sharedNeo4j.authToken, config); + const session = localDriver.session(); + const tx = session.beginTransaction(); + tx.run('RETURN 1').then(() => { + tx.rollback(); + session.close(); + done.fail('Query did not fail'); + }).catch(error => { + tx.rollback(); + session.close(); + + expect(error.code).toEqual(neo4j.error.SERVICE_UNAVAILABLE); + + // in some environments non-routable address results in immediate 'connection refused' error and connect + // timeout is not fired; skip message assertion for such cases, it is important for connect attempt to not hang + if (error.message.indexOf('Failed to establish connection') === 0) { + expect(error.message).toEqual('Failed to establish connection in 1000ms'); + } + + done(); + }); + } + }); From 6c1f5d1f60cde75dc3ecf6ac0a4d2285d124654e Mon Sep 17 00:00:00 2001 From: lutovich Date: Fri, 15 Sep 2017 00:15:14 +0200 Subject: [PATCH 2/2] Update dev version in package.json --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e4e289d0e..9fb7dea1a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "neo4j-driver", - "version": "1.2.0-dev", + "version": "1.4.1-dev", "description": "Connect to Neo4j 3.1.0 and up from JavaScript", "author": "Neo Technology Inc.", "license": "Apache-2.0",