Merge pull request #297 from lutovich/1.5-pool-configs

Deprecate connectionPoolSize config setting

Author: zhenlineo

src/v1/driver.js

@@ -25,6 +25,7 @@ import {newError, SERVICE_UNAVAILABLE} from './error';
 import {DirectConnectionProvider} from './internal/connection-providers';
 import Bookmark from './internal/bookmark';
 import ConnectivityVerifier from './internal/connectivity-verifier';
+import PoolConfig from './internal/pool-config';
 
 const READ = 'READ', WRITE = 'WRITE';
 /**
@@ -60,11 +61,7 @@ class Driver {
       this._createConnection.bind(this),
       this._destroyConnection.bind(this),
       this._validateConnection.bind(this),
-      {
-        maxIdleSize: config.connectionPoolSize,
-        maxSize: config.maxConnectionPoolSize,
-        acquisitionTimeout: config.connectionAcquisitionTimeout
-      }
+      PoolConfig.fromDriverConfig(config)
     );
 
     /**
@@ -243,18 +240,17 @@ class _ConnectionStreamObserver extends StreamObserver {
 function sanitizeConfig(config) {
   config.maxConnectionLifetime = sanitizeIntValue(config.maxConnectionLifetime);
   config.maxConnectionPoolSize = sanitizeIntValue(config.maxConnectionPoolSize);
-  config.connectionAcquisitionTimeout = sanitizeIntValue(config.connectionAcquisitionTimeout, 60000);
+  config.connectionAcquisitionTimeout = sanitizeIntValue(config.connectionAcquisitionTimeout);
 }
 
-function sanitizeIntValue(value, defaultValue=null) {
+function sanitizeIntValue(value) {
   if (value) {
       const sanitizedValue = parseInt(value, 10);
       if (sanitizedValue && sanitizedValue > 0) {
         return sanitizedValue;
       }
   }
-
-  return defaultValue;
+  return null;
 }
 
 export {Driver, READ, WRITE}

src/v1/index.js

@@ -90,7 +90,7 @@ const USER_AGENT = "neo4j-javascript/" + VERSION;
  *       // as trusted. In the web bundle, this list of trusted certificates is maintained
  *       // by the web browser. In NodeJS, you configure the list with the next config option.
  *       //
- *       // TRUST_SYSTEM_CA_SIGNED_CERTIFICATES meand that you trust whatever certificates
+ *       // TRUST_SYSTEM_CA_SIGNED_CERTIFICATES means that you trust whatever certificates
  *       // are in the default certificate chain of th
  *       trust: "TRUST_ALL_CERTIFICATES" | "TRUST_ON_FIRST_USE" | "TRUST_SIGNED_CERTIFICATES" |
  *       "TRUST_CUSTOM_CA_SIGNED_CERTIFICATES" | "TRUST_SYSTEM_CA_SIGNED_CERTIFICATES",
@@ -110,6 +110,7 @@ const USER_AGENT = "neo4j-javascript/" + VERSION;
  *
  *       // The max number of connections that are allowed idle in the pool at any time.
  *       // Connection will be destroyed if this threshold is exceeded.
+ *       // <b>Deprecated:</b> please use <code>maxConnectionPoolSize</code> instead.
  *       connectionPoolSize: 50,
  *
  *       // The maximum total number of connections allowed to be managed by the connection pool, per host.

src/v1/internal/ch-node.js

@@ -108,7 +108,7 @@ const TrustStrategy = {
    * @deprecated Since version 1.0. Will be deleted in a future version. {@link #TRUST_CUSTOM_CA_SIGNED_CERTIFICATES}.
    */
   TRUST_SIGNED_CERTIFICATES: function( config, onSuccess, onFailure ) {
-    console.log("`TRUST_SIGNED_CERTIFICATES` has been deprecated as option and will be removed in a future version of " +
+    console.warn('`TRUST_SIGNED_CERTIFICATES` has been deprecated as option and will be removed in a future version of ' +
       "the driver. Please use `TRUST_CUSTOM_CA_SIGNED_CERTIFICATES` instead.");
     return TrustStrategy.TRUST_CUSTOM_CA_SIGNED_CERTIFICATES(config, onSuccess, onFailure);
   },
@@ -172,7 +172,7 @@ const TrustStrategy = {
    * @deprecated in 1.1 in favour of {@link #TRUST_ALL_CERTIFICATES}. Will be deleted in a future version.
    */
   TRUST_ON_FIRST_USE : function( config, onSuccess, onFailure ) {
-      console.log("`TRUST_ON_FIRST_USE` has been deprecated as option and will be removed in a future version of " +
+    console.warn('`TRUST_ON_FIRST_USE` has been deprecated as option and will be removed in a future version of ' +
           "the driver. Please use `TRUST_ALL_CERTIFICATES` instead.");
 
     let tlsOpts = {

src/v1/internal/pool-config.js

@@ -0,0 +1,69 @@
+/**
+ * Copyright (c) 2002-2017 "Neo Technology,","
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * 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.
+ */
+
+const DEFAULT_MAX_SIZE = 50;
+const DEFAULT_ACQUISITION_TIMEOUT = 60000;
+
+export default class PoolConfig {
+
+  constructor(maxSize, acquisitionTimeout) {
+    this.maxSize = valueOrDefault(maxSize, DEFAULT_MAX_SIZE);
+    this.acquisitionTimeout = valueOrDefault(acquisitionTimeout, DEFAULT_ACQUISITION_TIMEOUT);
+  }
+
+  static defaultConfig() {
+    return new PoolConfig(DEFAULT_MAX_SIZE, DEFAULT_ACQUISITION_TIMEOUT);
+  }
+
+  static fromDriverConfig(config) {
+    const maxIdleSizeConfigured = isConfigured(config.connectionPoolSize);
+    const maxSizeConfigured = isConfigured(config.maxConnectionPoolSize);
+
+    let maxSize;
+
+    if (maxSizeConfigured) {
+      // correct size setting is set - use it's value
+      maxSize = config.maxConnectionPoolSize;
+    } else if (maxIdleSizeConfigured) {
+      // deprecated size setting is set - use it's value
+      console.warn('WARNING: neo4j-driver setting "connectionPoolSize" is deprecated, please use "maxConnectionPoolSize" instead');
+      maxSize = config.connectionPoolSize;
+    } else {
+      maxSize = DEFAULT_MAX_SIZE;
+    }
+
+    const acquisitionTimeoutConfigured = isConfigured(config.connectionAcquisitionTimeout);
+    const acquisitionTimeout = acquisitionTimeoutConfigured ? config.connectionAcquisitionTimeout : DEFAULT_ACQUISITION_TIMEOUT;
+
+    return new PoolConfig(maxSize, acquisitionTimeout);
+  }
+}
+
+function valueOrDefault(value, defaultValue) {
+  return value === 0 || value ? value : defaultValue;
+}
+
+function isConfigured(value) {
+  return value === 0 || value;
+}
+
+export {
+  DEFAULT_MAX_SIZE,
+  DEFAULT_ACQUISITION_TIMEOUT
+};

src/v1/internal/pool.js

@@ -17,27 +17,25 @@
  * limitations under the License.
  */
 
-import { newError } from "../error";
-import { promiseOrTimeout } from "./util";
+import {promiseOrTimeout} from './util';
+import PoolConfig from './pool-config';
 
 class Pool {
   /**
-   * @param create  an allocation function that creates a new resource. It's given
+   * @param {function} create  an allocation function that creates a new resource. It's given
    *                a single argument, a function that will return the resource to
    *                the pool if invoked, which is meant to be called on .dispose
    *                or .close or whatever mechanism the resource uses to finalize.
-   * @param destroy called with the resource when it is evicted from this pool
-   * @param validate called at various times (like when an instance is acquired and
+   * @param {function} destroy called with the resource when it is evicted from this pool
+   * @param {function} validate called at various times (like when an instance is acquired and
    *                 when it is returned). If this returns false, the resource will
    *                 be evicted
-   * @param maxIdle the max number of resources that are allowed idle in the pool at
-   *                any time. If this threshold is exceeded, resources will be evicted.
+   * @param {PoolConfig} config configuration for the new driver.
    */
-  constructor(create, destroy=(()=>true), validate=(()=>true), config={}) {
+  constructor(create, destroy = (() => true), validate = (() => true), config = PoolConfig.defaultConfig()) {
     this._create = create;
     this._destroy = destroy;
     this._validate = validate;
-    this._maxIdleSize = config.maxIdleSize;
     this._maxSize = config.maxSize;
     this._acquisitionTimeout = config.acquisitionTimeout;
     this._pools = {};
@@ -151,7 +149,7 @@ class Pool {
 
     if (pool) {
       // there exist idle connections for the given key
-      if (pool.length >= this._maxIdleSize || !this._validate(resource)) {
+      if (!this._validate(resource)) {
         this._destroy(resource);
       } else {
         pool.push(resource);

test/internal/pool-config.test.js

@@ -0,0 +1,110 @@
+/**
+ * Copyright (c) 2002-2017 "Neo Technology,","
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * 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 PoolConfig, {DEFAULT_ACQUISITION_TIMEOUT, DEFAULT_MAX_SIZE} from '../../src/v1/internal/pool-config';
+
+describe('PoolConfig', () => {
+
+  let originalConsoleWarn;
+
+  beforeAll(() => {
+    originalConsoleWarn = console.warn;
+    console.warn = () => {
+    };
+  });
+
+  afterAll(() => {
+    console.warn = originalConsoleWarn;
+  });
+
+  it('should respect zero values', () => {
+    const config = new PoolConfig(0, 0, 0);
+
+    expect(config.maxSize).toEqual(0);
+    expect(config.acquisitionTimeout).toEqual(0);
+  });
+
+  it('should expose default config', () => {
+    const config = PoolConfig.defaultConfig();
+
+    expect(config.maxSize).toEqual(DEFAULT_MAX_SIZE);
+    expect(config.acquisitionTimeout).toEqual(DEFAULT_ACQUISITION_TIMEOUT);
+  });
+
+  it('should convert from empty driver config', () => {
+    const driverConfig = {};
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(DEFAULT_MAX_SIZE);
+    expect(config.acquisitionTimeout).toEqual(DEFAULT_ACQUISITION_TIMEOUT);
+  });
+
+  it('should convert from full driver config', () => {
+    const driverConfig = {
+      maxConnectionPoolSize: 42,
+      connectionAcquisitionTimeout: 4242
+    };
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(42);
+    expect(config.acquisitionTimeout).toEqual(4242);
+  });
+
+  it('should convert from driver config with both connectionPoolSize and maxConnectionPoolSize', () => {
+    const driverConfig = {
+      connectionPoolSize: 42,
+      maxConnectionPoolSize: 4242
+    };
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(4242);
+    expect(config.acquisitionTimeout).toEqual(DEFAULT_ACQUISITION_TIMEOUT);
+  });
+
+  it('should convert from driver config without connectionPoolSize and maxConnectionPoolSize', () => {
+    const driverConfig = {
+      connectionAcquisitionTimeout: 42
+    };
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(DEFAULT_MAX_SIZE);
+    expect(config.acquisitionTimeout).toEqual(42);
+  });
+
+  it('should convert from driver config with only connectionPoolSize', () => {
+    const driverConfig = {
+      connectionPoolSize: 42
+    };
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(42);
+    expect(config.acquisitionTimeout).toEqual(DEFAULT_ACQUISITION_TIMEOUT);
+  });
+
+  it('should convert from driver config with only maxConnectionPoolSize', () => {
+    const driverConfig = {
+      maxConnectionPoolSize: 42
+    };
+    const config = PoolConfig.fromDriverConfig(driverConfig);
+
+    expect(config.maxSize).toEqual(42);
+    expect(config.acquisitionTimeout).toEqual(DEFAULT_ACQUISITION_TIMEOUT);
+  });
+
+});

test/internal/pool.test.js

@@ -18,6 +18,7 @@
  */
 
 import Pool from '../../src/v1/internal/pool';
+import PoolConfig from '../../src/v1/internal/pool-config';
 
 describe('Pool', () => {
 
@@ -103,44 +104,6 @@ describe('Pool', () => {
     });
   });
 
-  it('frees if pool reaches max size', (done) => {
-    // Given a pool that tracks destroyed resources
-    let counter = 0;
-    let destroyed = [];
-    const key = 'bolt://localhost:7687';
-    const pool = new Pool(
-      (url, release) => new Resource(url, counter++, release),
-      resource => {
-        destroyed.push(resource);
-      },
-      resource => true,
-      {
-        maxIdleSize: 2
-      }
-    );
-
-    // When
-    const p0 = pool.acquire(key);
-    const p1 = pool.acquire(key);
-    const p2 = pool.acquire(key);
-
-    // Then
-    Promise.all([ p0, p1, p2 ]).then(values => {
-      const r0 = values[0];
-      const r1 = values[1];
-      const r2 = values[2];
-
-      r0.close();
-      r1.close();
-      r2.close();
-
-      expect(destroyed.length).toBe(1);
-      expect(destroyed[0].id).toBe(r2.id);
-
-      done();
-    });
-  });
-
   it('frees if validate returns false', (done) => {
     // Given a pool that allocates
     let counter = 0;
@@ -152,9 +115,7 @@ describe('Pool', () => {
         destroyed.push(resource);
       },
       resource => false,
-      {
-        maxIdleSize: 1000
-      }
+      new PoolConfig(1000, 60000)
     );
 
     // When
@@ -440,10 +401,7 @@ describe('Pool', () => {
         (url, release) => new Resource(url, counter++, release),
         resource => {},
         resource => true,
-        {
-          maxSize: 2,
-          acquisitionTimeout: 5000
-        }
+      new PoolConfig(2, 5000)
     );
 
     const p0 = pool.acquire(key);
@@ -473,10 +431,7 @@ describe('Pool', () => {
         (url, release) => new Resource(url, counter++, release),
         resource => {},
         resource => true,
-        {
-          maxSize: 2,
-          acquisitionTimeout: 1000
-        }
+      new PoolConfig(2, 1000)
     );
 
     const p0 = pool.acquire(key);

test/internal/shared-neo4j.js

@@ -95,7 +95,7 @@ const password = 'password';
 const authToken = neo4j.auth.basic(username, password);
 
 const neoCtrlVersionParam = '-e';
-const defaultNeo4jVersion = '3.2.0';
+const defaultNeo4jVersion = '3.2.5';
 const defaultNeoCtrlArgs = `${neoCtrlVersionParam} ${defaultNeo4jVersion}`;
 
 function neo4jCertPath(dir) {