From f8b7a9ec2bd313c35276152f2fcee939c6e62871 Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Tue, 29 Dec 2015 11:57:07 +0300 Subject: [PATCH 1/8] Fixed issue when connection object is not released after error or timeout --- lib/index.coffee | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/index.coffee b/lib/index.coffee index f9ffa99..504fbc6 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -106,12 +106,18 @@ module.exports = (thrift, service, pool_options = {}, thrift_options = {}) -> cb = _.once cb cb_error = (err) -> debug "in error callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb err cb_timeout = -> debug "in timeout callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb new Error TIMEOUT_MESSAGE cb_close = -> debug "in close callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb new Error CLOSE_MESSAGE add_listeners connection, cb_error, cb_timeout, cb_close client = thrift.createClient service, connection From 7287b6a67b5d8790eec9bc4c09da69bbb40bdfb4 Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Tue, 29 Dec 2015 12:47:22 +0300 Subject: [PATCH 2/8] Added SSL support --- README.md | 1 + lib/index.coffee | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 4ed730d..58c0dbb 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,7 @@ Options to use when creating pool, defaults match those used by node-pool. - `max_connections` - Default: `1` - Max number of connections to keep open at any given time - `min_connections` - Default: `0` - Min number of connections to keep open at any given time - `idle_timeout` - Default: `30000` - Time (ms) to wait until closing idle connections +- `ssl` - Default: undefined - If the option is passed SSL/TLS connection will be used. ## Thrift options - optional All thrift options are supported and can be passed in as an object in addition to the pooling options. diff --git a/lib/index.coffee b/lib/index.coffee index 504fbc6..539d7e0 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -13,7 +13,10 @@ CLOSE_MESSAGE = "Thrift-pool: Connection closed" # @param thrift_options, passed to thrift connection, create_cb = (thrift, pool_options, thrift_options, cb) -> cb = _.once cb - connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options + if pool_options.ssl? + connection = thrift.createSSLConnection pool_options.host, pool_options.port, thrift_options + else + connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options connection.__ended = false connection.on "connect", -> debug "in connect callback" From ec6cebf159e0e3cd5a521788bfeee279d8ef18e1 Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Sat, 9 Jan 2016 10:00:25 +0300 Subject: [PATCH 3/8] boolean ssl option --- README.md | 2 +- lib/index.coffee | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 58c0dbb..f9fb978 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ Options to use when creating pool, defaults match those used by node-pool. - `max_connections` - Default: `1` - Max number of connections to keep open at any given time - `min_connections` - Default: `0` - Min number of connections to keep open at any given time - `idle_timeout` - Default: `30000` - Time (ms) to wait until closing idle connections -- `ssl` - Default: undefined - If the option is passed SSL/TLS connection will be used. +- `ssl` - Default: `false` - If true SSL/TLS connection will be used. ## Thrift options - optional All thrift options are supported and can be passed in as an object in addition to the pooling options. diff --git a/lib/index.coffee b/lib/index.coffee index 539d7e0..da84071 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -13,7 +13,8 @@ CLOSE_MESSAGE = "Thrift-pool: Connection closed" # @param thrift_options, passed to thrift connection, create_cb = (thrift, pool_options, thrift_options, cb) -> cb = _.once cb - if pool_options.ssl? + pool_options.ssl ?= false + if pool_options.ssl connection = thrift.createSSLConnection pool_options.host, pool_options.port, thrift_options else connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options From 68c27a2a339235cd8febeb1e68a9852635f5a25f Mon Sep 17 00:00:00 2001 From: Alex Zylman Date: Tue, 2 Feb 2016 19:27:10 -0800 Subject: [PATCH 4/8] add ttl option to connections in the pool --- README.md | 17 +++++++++++++++++ lib/index.coffee | 6 +++++- test/unit.coffee | 13 ++++++++++++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4ed730d..20720a4 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,23 @@ Options to use when creating pool, defaults match those used by node-pool. - `max_connections` - Default: `1` - Max number of connections to keep open at any given time - `min_connections` - Default: `0` - Min number of connections to keep open at any given time - `idle_timeout` - Default: `30000` - Time (ms) to wait until closing idle connections +- `ttl` - Default: `undefined` - Time (+/-50%) (ms) past which to destroy the connection. For more explanation, see below + +### ttl + +When using load balancers such as Amazon's ELB service, a common deploy strategy is to deploy new instances with new versions of your code, attach them to the load balancer, and detach the old ones. +This doesn't work very well with persistent connections, however. +New connections go to the new servers, but any open connections to the old servers remain and are eventually closed by the server when the servers turn off. +This causes any in-flight requests on those connections to error with `Error: Thrift-pool: Connection closed`. + +The `ttl` option treats any connection older than `ttl` (+/-50%) as invalid, so if something tries to acquite it, they'll get a more recent connection instead. +If you set this time lower than the [connection drain timeout](http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/config-conn-drain.html) of your load balancer, you can guarantee that all connections to the old servers are closed before the servers are stopped. + +We use some amount of randomness (+/- 50%) of this time to ensure that all of your connections are not marked invalid simultaneously. + +As an example, we have our connection drain timeout set to five minutes, so when you're removing instances from the ELB, any connections older than five minutes are just cut off. +Based on that, we set our `ttl` to two minutes, and guarantee that all connections will be destroyed sometime after one to three minutes. + ## Thrift options - optional All thrift options are supported and can be passed in as an object in addition to the pooling options. diff --git a/lib/index.coffee b/lib/index.coffee index f9ffa99..37f3efc 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -15,6 +15,8 @@ create_cb = (thrift, pool_options, thrift_options, cb) -> cb = _.once cb connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options connection.__ended = false + if pool_options.ttl? + connection.__reap_time = Date.now() + _.random (pool_options.ttl / 2), (pool_options.ttl * 1.5) connection.on "connect", -> debug "in connect callback" connection.connection.setKeepAlive(true) @@ -50,7 +52,9 @@ create_pool = (thrift, pool_options = {}, thrift_options = {}) -> connection.end() validate: (connection) -> debug "in validate" - not connection.__ended + return false if connection.__ended + return true unless pool_options.ttl? + connection.__reap_time < Date.now() log: pool_options.log max: pool_options.max_connections min: pool_options.min_connections diff --git a/test/unit.coffee b/test/unit.coffee index ff289cf..2f03abe 100644 --- a/test/unit.coffee +++ b/test/unit.coffee @@ -279,18 +279,29 @@ describe 'create_pool unit', -> # - connection is valid, it is returned # - connection is not valid, it is destroyed, new connection is returned. it 'properly validates a connection', (done) -> - pool = _private.create_pool @thrift, @options + pool = _private.create_pool @thrift, _.extend {ttl: 100}, @options async.series [ (cb) => # Connection is valid and is released @mock_connection.end.reset() setImmediate => @mock_connection.emit "connect" @thrift.createConnection.reset() + @mock_connection.__reap_time = Date.now() - 1 pool.acquire (err, connection) => @assert_valid err, connection assert.equal @mock_connection.end.called, false pool.release connection cb() + (cb) => + # Connection is invalid due to TTL and is destroyed and a + # new connection is created and returned + @mock_connection.end.reset() + setImmediate => @mock_connection.emit "connect" + @thrift.createConnection.reset() + @mock_connection.__reap_time = Date.now() + 1 + assert.equal pool.getPoolSize(), 1 + assert.equal pool.availableObjectsCount(), 1 + @acquire_destroys pool, cb (cb) => # Connection in pool is marked invalid, destroyed, and a # new connection is created and returned From f0f89eac7091e2e1f9d21511091f9521900dfc7f Mon Sep 17 00:00:00 2001 From: Alex Zylman Date: Wed, 3 Feb 2016 18:43:35 +0000 Subject: [PATCH 5/8] 1.2.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 40c3051..6aaaa07 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "node-thrift-pool", - "version": "1.1.0", + "version": "1.2.0", "description": "A module that wraps thrift interfaces in connection pooling logic to make them more resilient.", "main": "index.js", "scripts": { From 2b2560ff9a2d0a5a869558c2a72a1ce79af5a7ff Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Tue, 29 Dec 2015 11:57:07 +0300 Subject: [PATCH 6/8] Fixed issue when connection object is not released after error or timeout --- lib/index.coffee | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/index.coffee b/lib/index.coffee index 37f3efc..e091784 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -110,12 +110,18 @@ module.exports = (thrift, service, pool_options = {}, thrift_options = {}) -> cb = _.once cb cb_error = (err) -> debug "in error callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb err cb_timeout = -> debug "in timeout callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb new Error TIMEOUT_MESSAGE cb_close = -> debug "in close callback, post-acquire listener" + remove_listeners connection, cb_error, cb_timeout, cb_close + pool.release connection cb new Error CLOSE_MESSAGE add_listeners connection, cb_error, cb_timeout, cb_close client = thrift.createClient service, connection From 67e3942e0c4a9d2744a4421212797bffe02014ef Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Tue, 29 Dec 2015 12:47:22 +0300 Subject: [PATCH 7/8] Added SSL support --- README.md | 2 ++ lib/index.coffee | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 20720a4..bf377e8 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,7 @@ Options to use when creating pool, defaults match those used by node-pool. - `min_connections` - Default: `0` - Min number of connections to keep open at any given time - `idle_timeout` - Default: `30000` - Time (ms) to wait until closing idle connections - `ttl` - Default: `undefined` - Time (+/-50%) (ms) past which to destroy the connection. For more explanation, see below +- `ssl` - Default: `false` - If true, SSL/TLS connection will be used. ### ttl @@ -57,6 +58,7 @@ We use some amount of randomness (+/- 50%) of this time to ensure that all of yo As an example, we have our connection drain timeout set to five minutes, so when you're removing instances from the ELB, any connections older than five minutes are just cut off. Based on that, we set our `ttl` to two minutes, and guarantee that all connections will be destroyed sometime after one to three minutes. +======= ## Thrift options - optional All thrift options are supported and can be passed in as an object in addition to the pooling options. diff --git a/lib/index.coffee b/lib/index.coffee index e091784..68cf7fd 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -13,7 +13,10 @@ CLOSE_MESSAGE = "Thrift-pool: Connection closed" # @param thrift_options, passed to thrift connection, create_cb = (thrift, pool_options, thrift_options, cb) -> cb = _.once cb - connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options + if pool_options.ssl? + connection = thrift.createSSLConnection pool_options.host, pool_options.port, thrift_options + else + connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options connection.__ended = false if pool_options.ttl? connection.__reap_time = Date.now() + _.random (pool_options.ttl / 2), (pool_options.ttl * 1.5) From 82755d9a1d4ea2868c55671503c0b07dd66fad07 Mon Sep 17 00:00:00 2001 From: Dmitriy Chugunov Date: Sat, 9 Jan 2016 10:00:25 +0300 Subject: [PATCH 8/8] boolean ssl option --- lib/index.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/index.coffee b/lib/index.coffee index 68cf7fd..5d10600 100644 --- a/lib/index.coffee +++ b/lib/index.coffee @@ -13,7 +13,8 @@ CLOSE_MESSAGE = "Thrift-pool: Connection closed" # @param thrift_options, passed to thrift connection, create_cb = (thrift, pool_options, thrift_options, cb) -> cb = _.once cb - if pool_options.ssl? + pool_options.ssl ?= false + if pool_options.ssl connection = thrift.createSSLConnection pool_options.host, pool_options.port, thrift_options else connection = thrift.createConnection pool_options.host, pool_options.port, thrift_options