Feature: Add internal cache of Service Client instances (#8)

* Update changelog

* Add client instances object to global config

* Check for cached instance of service

* Add destroy method to remove service from cache

* Remove return

* Fix tests

* Change destroy to remove for service cache

* Update logic for overrides

* Gramar fix

* Updated tests to provide a check against cached instances

* Update README, CHANGELOG, redundant overrides check

Author: cmoody

CHANGELOG.md

@@ -1,6 +1,13 @@
-## [1.2.1](https://github.com/expediagroup/service-client/compare/v1.2.0...v1.2.1) (2019-08-22)
+## [1.3.0](https://github.com/expediagroup/service-client/compare/v1.2.1...v1.3.0) (2019-12-03)
+
+#### Feature
+
+* Caches a client if one does not already exist
+* Returns a cached client if already generated
 
 
+## [1.2.1](https://github.com/expediagroup/service-client/compare/v1.2.0...v1.2.1) (2019-08-22)
+
 #### Bug Fixes
 
 * add hook debug statements ([#5](https://github.com/expediagroup/service-client/issues/5)) ([647534c](https://github.com/expediagroup/service-client/commit/647534c))

README.md

@@ -94,6 +94,8 @@ For a more thorough collection of examples see the [examples directory](https://
 
 Returns a new service client instance for `servicename` with optional `overrides` to the global defaults listed above:
 
+*Note: If no `overrides` are provided, when a service client instance is created for a `servicename` it will be stored in a cache.  That instance will be returned instead of creating a new instance.*
+
 - **protocol** - The protocol to use for the request. Defaults to `"http:"`.
 - **hostname** - The hostname to use for the request. Accepts a `string` or a `function(serviceName, serviceConfig)` that returns a string.
 - **port** - The port number to use for the request.

lib/config.js

@@ -36,6 +36,8 @@ const globalConfig = individual('__serviceclientconfig', {
       keepAliveMsecs: 30000
     }
   },
+  // cached client instances
+  clientInstances: {},
   overrides: {
     // service specific overrides
     // consulkey: { protocol, host, port }

lib/index.js

@@ -38,11 +38,27 @@ const create = function (servicename, overrides = {}) {
     config.agent = Agent.create(config.protocol, config)
   }
 
-  const client = new Client(servicename, config)
+  // If overrides are passed do not cache the client
+  if (Object.keys(overrides).length) {
+    const client = new Client(servicename, config)
 
-  debug('create(): created client id %s for service %s', client.id, servicename)
+    debug('create(): created client id %s for service %s', client.id, servicename)
 
-  return client
+    return client
+  }
+
+  // If the client is not already cached create an instance and cache
+  if (typeof GlobalConfig.clientInstances[servicename] === 'undefined') {
+    GlobalConfig.clientInstances[servicename] = new Client(servicename, config)
+  }
+
+  debug('create(): created client id %s for service %s', GlobalConfig.clientInstances[servicename].id, servicename)
+
+  return GlobalConfig.clientInstances[servicename]
+}
+
+const remove = function (servicename) {
+  delete GlobalConfig.clientInstances[servicename]
 }
 
 const use = function (plugins = []) {
@@ -77,6 +93,7 @@ const mergeConfig = function (externalConfig = {}) {
 // Exported API
 const ServiceClient = {
   create,
+  remove,
   read: Read,
   use,
   mergeConfig

test/unit/client.test.js

@@ -89,6 +89,7 @@ describe('client', function () {
     afterEach(() => {
       Object.assign(__serviceclientconfig, suite.originalConfig) // eslint-disable-line no-undef
       process.env = suite.originalEnvVars
+      ServiceClient.remove('myservice')
     })
 
     it('should throw an error if creating an instance without a service name', async () => {
@@ -194,9 +195,26 @@ describe('client', function () {
       const SC = require('../../lib/client')
       assert.throws(() => new SC(), 'A service name is required')
     })
+
+    it('should use a cached instance when no overrides provided', () => {
+      __serviceclientconfig.overrides.cachedservice = { hostname: 'cachedservice.service.local' } // eslint-disable-line no-undef
+
+      const client1 = ServiceClient.create('cachedservice')
+      const client2 = ServiceClient.create('cachedservice', {})
+      const client3 = ServiceClient.create('cachedservice', {
+        basePath: '/v1/test'
+      })
+
+      assert.equal(client1, client2, 'client2 is equal to client1 because its pulled from the cache')
+      assert.notEqual(client1, client3, 'client3 provides an override and is a new instance of service client')
+    })
   })
 
   describe('service-client request', () => {
+    afterEach(() => {
+      ServiceClient.remove('myservice')
+    })
+
     it('should throw an error if requesting without an `operation`', async function () {
       const client = ServiceClient.create('myservice', { hostname: 'vrbo.com' })
 
@@ -330,6 +348,10 @@ describe('client', function () {
   })
 
   describe('service-client read', () => {
+    afterEach(() => {
+      ServiceClient.remove('myservice')
+    })
+
     it('should read the response payload', async function () {
       Nock('http://myservice.service.local:80')
         .get('/v1/test/stuff')
@@ -346,6 +368,10 @@ describe('client', function () {
   })
 
   describe('connectTimeout and circuit breaker', () => {
+    afterEach(() => {
+      ServiceClient.remove('myservice')
+    })
+
     it('should fail the request because `connectTimeout` is short', async function () {
       const client = ServiceClient.create('myservice', { hostname: 'myservice.service.local', connectTimeout: 1, maxFailures: 1 })
 
@@ -409,6 +435,10 @@ describe('client', function () {
   })
 
   describe('timeout and circuit breaker', () => {
+    afterEach(() => {
+      ServiceClient.remove('myservice')
+    })
+
     it('should fail the request because `timeout` is short and the circuit breaker should trip', async function () {
       Nock('http://myservice.service.local:80')
         .get('/v1/test/stuff')
@@ -438,6 +468,10 @@ describe('client', function () {
   })
 
   describe('circuit breaker and hooks', () => {
+    afterEach(() => {
+      ServiceClient.remove('myservice')
+    })
+
     it('should execute `error`, `stats`, and `end` hooks if the circuit breaker is open', async function () {
       Nock('http://myservice.service.local:80')
         .get('/v1/test/stuff')

test/unit/hooks/request-context.test.js

@@ -43,6 +43,7 @@ describe('Using ServiceClient in the context of a Hapi request', () => {
     GlobalConfig.plugins = []
     suite.sandbox.restore()
     suite = null
+    ServiceClient.remove('myservice')
   })
 
   it('should process requests that may be in flight simultaneously', async function () {

test/unit/hooks/server-context.test.js

@@ -31,6 +31,7 @@ describe('Using ServiceClient in the context of a Hapi server', () => {
     GlobalConfig.plugins = []
     suite.sandbox.restore()
     suite = null
+    ServiceClient.remove('myservice')
   })
 
   it('should pass the server via `context` to the `request` hook', async function () {

test/unit/hooks/standalone.test.js

@@ -29,6 +29,7 @@ describe('Using ServiceClient in a standalone context', () => {
     GlobalConfig.plugins = []
     suite.sandbox.restore()
     suite = null
+    ServiceClient.remove('myservice')
   })
 
   it('should call hooks (successful request)', async function () {