Do not add seed router to the initial routing table

Author: ali-ince

src/v1/internal/connection-providers.js

@@ -64,7 +64,7 @@ export class LoadBalancer extends ConnectionProvider {
   constructor(address, routingContext, connectionPool, loadBalancingStrategy, hostNameResolver, driverOnErrorCallback, log) {
     super();
     this._seedRouter = address;
-    this._routingTable = new RoutingTable([this._seedRouter]);
+    this._routingTable = new RoutingTable();
     this._rediscovery = new Rediscovery(new RoutingUtil(routingContext));
     this._connectionPool = connectionPool;
     this._driverOnErrorCallback = driverOnErrorCallback;
@@ -259,11 +259,8 @@ export class LoadBalancer extends ConnectionProvider {
   }
 
   _updateRoutingTable(newRoutingTable) {
-    const currentRoutingTable = this._routingTable;
-
     // close old connections to servers not present in the new routing table
-    const staleServers = currentRoutingTable.serversDiff(newRoutingTable);
-    staleServers.forEach(server => this._connectionPool.purge(server));
+    this._connectionPool.keepAll(newRoutingTable.allServers());
 
     // make this driver instance aware of the new table
     this._routingTable = newRoutingTable;

src/v1/internal/pool.js

@@ -115,6 +115,17 @@ class Pool {
     Object.keys(this._pools).forEach(key => this._purgeKey(key));
   }
 
+  /**
+   * Keep the idle resources for the provided addresses and purge the rest.
+   */
+  keepAll(addresses) {
+    const keysToKeep = addresses.map(a => a.asKey());
+    const keysPresent = Object.keys(this._pools);
+    const keysToPurge = keysPresent.filter(k => keysToKeep.indexOf(k) == -1);
+
+    keysToPurge.forEach(key => this._purgeKey(key));
+  }
+
   /**
    * Check if this pool contains resources for the given address.
    * @param {ServerAddress} address the address of the server to check.

src/v1/internal/routing-table.js

@@ -47,15 +47,6 @@ export default class RoutingTable {
     this.writers = removeFromArray(this.writers, address);
   }
 
-  serversDiff(otherRoutingTable) {
-    const oldServers = this._allServers();
-    const newServers = otherRoutingTable._allServers();
-    const diffTable = {};
-    oldServers.forEach(oldServer => diffTable[oldServer.asKey()] = oldServer);
-    newServers.forEach(newServer => delete diffTable[newServer.asKey()]);
-    return Object.values(diffTable);
-  }
-
   /**
    * Check if this routing table is fresh to perform the required operation.
    * @param {string} accessMode the type of operation. Allowed values are {@link READ} and {@link WRITE}.
@@ -68,7 +59,7 @@ export default class RoutingTable {
       accessMode === WRITE && this.writers.length === 0;
   }
 
-  _allServers() {
+  allServers() {
     return [...this.routers, ...this.readers, ...this.writers];
   }
 

test/internal/connection-providers.test.js

@@ -174,7 +174,7 @@ describe('LoadBalancer', () => {
       NO_OP_DRIVER_CALLBACK, Logger.noOp());
 
     expectRoutingTable(loadBalancer,
-      [serverABC],
+      [],
       [],
       []
     );

test/internal/node/routing.driver.boltkit.test.js

@@ -562,6 +562,8 @@ describe('routing driver with stub server', () => {
       // When
       const session1 = driver.session(neo4j.session.READ);
       session1.run("MATCH (n) RETURN n.name").catch(() => {
+        const session2 = driver.session(neo4j.session.READ);
+        session2.run('MATCH (n) RETURN n.name').then(() => {
           driver.close();
           seedServer.exit(code1 => {
             readServer.exit(code2 => {
@@ -570,6 +572,7 @@ describe('routing driver with stub server', () => {
               done();
             });
           });
+        });
       });
     });
   });
@@ -2131,13 +2134,13 @@ describe('routing driver with stub server', () => {
     // the first seed to get the routing table
     // the returned routing table includes a non-reachable read-server and points to only one router
     // which will return an invalid routing table
-    const seedServer1 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_point_to_empty_router.script', 9001);
+    const router1 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_point_to_empty_router_and_exit.script', 9001);
     // returns an empty routing table
-    const failingSeedServer2 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_empty.script', 9004);
+    const router2 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_empty.script', 9004);
     // returns a normal routing table
-    const seedServer3 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_three_servers.script', 9003);
+    const router3 = boltStub.start('./test/resources/boltstub/acquire_endpoints_v3_three_servers_and_exit.script', 9003);
     // ordinary read server
-    const readServer = boltStub.start('./test/resources/boltstub/read_server_v3_read_tx.script', 9002);
+    const reader = boltStub.start('./test/resources/boltstub/read_server_v3_read_tx.script', 9002);
 
     boltStub.run(() => {
       const driver = boltStub.newDriver('bolt+routing://my.virtual.host:8080', {
@@ -2148,10 +2151,10 @@ describe('routing driver with stub server', () => {
       session.readTransaction(tx => tx.run('MATCH (n) RETURN n.name')).then(res => {
         session.close();
         driver.close();
-        seedServer1.exit(code1 => {
-          failingSeedServer2.exit(code2 => {
-            seedServer3.exit(code3 => {
-              readServer.exit(code4 => {
+        router1.exit(code1 => {
+          router2.exit(code2 => {
+            router3.exit(code3 => {
+              reader.exit(code4 => {
                 expect(code1).toEqual(0);
                 expect(code2).toEqual(0);
                 expect(code3).toEqual(0);

test/internal/pool.test.js

@@ -296,6 +296,83 @@ describe('Pool', () => {
     });
   });
 
+  it('purges keys other than the ones to keep', done => {
+    let counter = 0;
+
+    const address1 = ServerAddress.fromUrl('bolt://localhost:7687');
+    const address2 = ServerAddress.fromUrl('bolt://localhost:7688');
+    const address3 = ServerAddress.fromUrl('bolt://localhost:7689');
+
+    const pool = new Pool((server, release) => Promise.resolve(new Resource(server, counter++, release)),
+      res => {
+        res.destroyed = true;
+        return true;
+      }
+    );
+
+    const acquiredResources = [
+      pool.acquire(address1),
+      pool.acquire(address2),
+      pool.acquire(address3),
+      pool.acquire(address1),
+      pool.acquire(address2),
+      pool.acquire(address3)
+    ];
+
+    Promise.all(acquiredResources).then(values => {
+      expect(pool.has(address1)).toBeTruthy();
+      expect(pool.has(address2)).toBeTruthy();
+      expect(pool.has(address3)).toBeTruthy();
+
+      pool.keepAll([address1, address3]);
+
+      expect(pool.has(address1)).toBeTruthy();
+      expect(pool.has(address3)).toBeTruthy();
+      expect(pool.has(address2)).toBeFalsy();
+
+      done();
+    });
+  });
+
+  it('purges all keys if addresses to keep is empty', done => {
+    let counter = 0;
+
+    const address1 = ServerAddress.fromUrl('bolt://localhost:7687');
+    const address2 = ServerAddress.fromUrl('bolt://localhost:7688');
+    const address3 = ServerAddress.fromUrl('bolt://localhost:7689');
+
+    const pool = new Pool((server, release) => Promise.resolve(new Resource(server, counter++, release)),
+      res => {
+        res.destroyed = true;
+        return true;
+      }
+    );
+
+    const acquiredResources = [
+      pool.acquire(address1),
+      pool.acquire(address2),
+      pool.acquire(address3),
+      pool.acquire(address1),
+      pool.acquire(address2),
+      pool.acquire(address3)
+    ];
+
+    Promise.all(acquiredResources).then(values => {
+      expect(pool.has(address1)).toBeTruthy();
+      expect(pool.has(address2)).toBeTruthy();
+      expect(pool.has(address3)).toBeTruthy();
+
+      pool.keepAll([]);
+
+      expect(pool.has(address1)).toBeFalsy();
+      expect(pool.has(address3)).toBeFalsy();
+      expect(pool.has(address2)).toBeFalsy();
+
+      done();
+    });
+  });
+
+
   it('skips broken connections during acquire', (done) => {
     let validated = false;
     let counter = 0;

test/internal/rediscovery.test.js

@@ -180,7 +180,7 @@ describe('rediscovery', () => {
 
       expect(routingTable.expirationTime).toEqual(expires);
 
-      const allServers = routingTable.serversDiff(new RoutingTable()).sort();
+      const allServers = routingTable.allServers().sort();
       const allExpectedServers = [...routerAddresses, ...readerAddresses, ...writerAddresses].sort();
       expect(allServers.map(s => s.asHostPort())).toEqual(allExpectedServers);
 

test/internal/routing-table.test.js

@@ -127,69 +127,6 @@ describe('routing-table', () => {
     expect(table.writers).toEqual([server5]);
   });
 
-  it('should return all servers in diff when other table is empty', () => {
-    const oldTable = createTable([server1, server2], [server3, server4], [server5, server6], notExpired());
-    const newTable = createTable([], [], [], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server1, server2, server3, server4, server5, server6]);
-  });
-
-  it('should no servers in diff when this table is empty', () => {
-    const oldTable = createTable([], [], [], notExpired());
-    const newTable = createTable([server1, server2], [server3, server4], [server5, server6], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([]);
-  });
-
-  it('should include different routers in servers diff', () => {
-    const oldTable = createTable([server1, server7, server2, server42], [server3, server4], [server5, server6], notExpired());
-    const newTable = createTable([server1, server2], [server3, server4], [server5, server6], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server7, server42]);
-  });
-
-  it('should include different readers in servers diff', () => {
-    const oldTable = createTable([server1, server2], [server3, server7, server4, server42], [server5, server6], notExpired());
-    const newTable = createTable([server1, server2], [server3, server4], [server5, server6], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server7, server42]);
-  });
-
-  it('should include different writers in servers diff', () => {
-    const oldTable = createTable([server1, server2], [server3, server4], [server5, server7, server6, server42], notExpired());
-    const newTable = createTable([server1, server2], [server3, server4], [server5, server6], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server7, server42]);
-  });
-
-  it('should include different servers in diff', () => {
-    const oldTable = createTable([server1, server2, server11], [server22, server3, server33, server4], [server5, server44, server6], notExpired());
-    const newTable = createTable([server1], [server2, server3, server4, server6], [server5], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server11, server22, server33, server44]);
-  });
-
-  it('should include different servers in diff with logical equality', () => {
-    const oldTable = createTable([server1, server11], [server2, server22], [server3, server33], notExpired());
-    const newTable = createTable([ServerAddress.fromUrl(server1.asHostPort())], [ServerAddress.fromUrl(server2.asHostPort())], [ServerAddress.fromUrl(server3.asHostPort())], notExpired());
-
-    const servers = oldTable.serversDiff(newTable);
-
-    expect(servers).toEqual([server11, server22, server33]);
-  });
-
   it('should have correct toString', () => {
     const originalDateNow = Date.now;
     try {