From 8d7fe2bb68733b10e4aba8e65a655ec1966cac93 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:33:35 -0500 Subject: [PATCH 01/18] Storing `path`, `stream` and `udsGracefulRestartRateLimit` on `client` in constructor. Also using these in places where `options` was used. --- lib/statsd.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index b1af358..11ad6e4 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -48,6 +48,8 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, this.cacheDnsTtl = options.cacheDnsTtl || CACHE_DNS_TTL_DEFAULT; this.host = options.host || process.env.DD_AGENT_HOST; this.port = options.port || parseInt(process.env.DD_DOGSTATSD_PORT, 10) || 8125; + this.path = options.path; + this.stream = options.stream; this.prefix = options.prefix || ''; this.suffix = options.suffix || ''; this.tagPrefix = options.tagPrefix || '#'; @@ -68,6 +70,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, this.bufferHolder = options.isChild ? options.bufferHolder : { buffer: '' }; this.errorHandler = options.errorHandler; this.udsGracefulErrorHandling = 'udsGracefulErrorHandling' in options ? options.udsGracefulErrorHandling : true; + this.udsGracefulRestartRateLimit = options.udsGracefulRestartRateLimit || UDS_DEFAULT_GRACEFUL_RESTART_LIMIT; // only recreate once per second // If we're mocking the client, create a buffer to record the outgoing calls. if (this.mock) { @@ -99,10 +102,10 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, host: this.host, cacheDns: this.cacheDns, cacheDnsTtl: this.cacheDnsTtl, - path: options.path, + path: this.path, port: this.port, protocol: this.protocol, - stream: options.stream + stream: this.stream }); } @@ -120,7 +123,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, // options.udsGracefulRestartRateLimit is the minimum time (ms) between creating sockets // does not support options.isChild (how to re-create a socket you didn't create?) if (this.socket && !options.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { - const socketCreateLimit = options.udsGracefulRestartRateLimit || UDS_DEFAULT_GRACEFUL_RESTART_LIMIT; // only recreate once per second + const socketCreateLimit = this.udsGracefulRestartRateLimit; const lastSocketCreateTime = Date.now(); this.socket.on('error', (err) => { const code = err.code; @@ -135,7 +138,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, this.socket.close(); this.socket = createTransport(this, { host: this.host, - path: options.path, + path: this.path, port: this.port, protocol: this.protocol }); From 5eb2e1934945b86010a85c3f90426b53b3a89bae Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:34:26 -0500 Subject: [PATCH 02/18] Getting rid of `socketCreateLimit` local (not needed any longer). --- lib/statsd.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index 11ad6e4..c130a27 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -123,14 +123,13 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, // options.udsGracefulRestartRateLimit is the minimum time (ms) between creating sockets // does not support options.isChild (how to re-create a socket you didn't create?) if (this.socket && !options.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { - const socketCreateLimit = this.udsGracefulRestartRateLimit; const lastSocketCreateTime = Date.now(); this.socket.on('error', (err) => { const code = err.code; switch (code) { case 107: case 111: { - if (Date.now() - lastSocketCreateTime >= socketCreateLimit) { + if (Date.now() - lastSocketCreateTime >= this.udsGracefulRestartRateLimit) { // recreate the socket, but only once per 30 seconds if (this.errorHandler) { this.socket.removeListener('error', this.errorHandler); From 700fb83d8f2331611f5c9b8a56088a837be8fa13 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:35:39 -0500 Subject: [PATCH 03/18] Introduce `trySetNewSocket()` helper. This way we can retry using this helper. --- lib/statsd.js | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index c130a27..7695d2b 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -98,15 +98,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, } if (! this.socket) { - this.socket = createTransport(this, { - host: this.host, - cacheDns: this.cacheDns, - cacheDnsTtl: this.cacheDnsTtl, - path: this.path, - port: this.port, - protocol: this.protocol, - stream: this.stream - }); + trySetNewSocket(this); } if (this.socket && !options.isChild && options.errorHandler) { @@ -528,3 +520,15 @@ Client.prototype.childClient = function (options) { exports = module.exports = Client; exports.StatsD = Client; + +function trySetNewSocket(client) { + client.socket = createTransport(client, { + host: client.host, + cacheDns: client.cacheDns, + cacheDnsTtl: client.cacheDnsTtl, + path: client.path, + port: client.port, + protocol: client.protocol, + stream: client.stream, + }); +} From ccc83e40bf4682f35e974ce97b81d8e777dab024 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:38:45 -0500 Subject: [PATCH 04/18] Replacing `switch` statement with `UDS_ERROR_CODES` constant. This is in advance of a change that will allow these to be OS specific. --- lib/constants.js | 6 ++++++ lib/statsd.js | 13 ++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/constants.js b/lib/constants.js index 1cc21af..d6760a3 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -4,3 +4,9 @@ exports.PROTOCOL = { UDP: 'udp', STREAM: 'stream' }; + +function udsErrors() { + return [107, 111]; +}; + +exports.udsErrors = udsErrors; diff --git a/lib/statsd.js b/lib/statsd.js index 7695d2b..d9cdfc8 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -2,9 +2,11 @@ const util = require('util'), helpers = require('./helpers'), applyStatsFns = require('./statsFunctions'); -const { PROTOCOL } = require('./constants'); +const constants = require('./constants'); const createTransport = require('./transport'); +const PROTOCOL = constants.PROTOCOL; +const UDS_ERROR_CODES = constants.udsErrors(); const UDS_DEFAULT_GRACEFUL_RESTART_LIMIT = 1000; const CACHE_DNS_TTL_DEFAULT = 60000; @@ -118,9 +120,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, const lastSocketCreateTime = Date.now(); this.socket.on('error', (err) => { const code = err.code; - switch (code) { - case 107: - case 111: { + if (UDS_ERROR_CODES.includes(code)) { if (Date.now() - lastSocketCreateTime >= this.udsGracefulRestartRateLimit) { // recreate the socket, but only once per 30 seconds if (this.errorHandler) { @@ -140,11 +140,6 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, this.socket.on('error', error => console.error(`hot-shots UDS error: ${error}`)); } } - break; - } - default: { - break; - } } }); } From eef577c9883c560670be191e53d6d23f6d14ee43 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:40:18 -0500 Subject: [PATCH 05/18] Using `-code` for check within `UDS_ERROR_CODES`. See https://nodejs.org/docs/latest-v12.x/api/errors.html for an example (in `error.errno`) explaining why error codes are negative. --- lib/statsd.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index d9cdfc8..65592dd 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -119,8 +119,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, if (this.socket && !options.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { const lastSocketCreateTime = Date.now(); this.socket.on('error', (err) => { - const code = err.code; - if (UDS_ERROR_CODES.includes(code)) { + if (UDS_ERROR_CODES.includes(-err.code)) { if (Date.now() - lastSocketCreateTime >= this.udsGracefulRestartRateLimit) { // recreate the socket, but only once per 30 seconds if (this.errorHandler) { From 9db6aa01bf4c5c375d080f3d617e161e5069f663 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:41:33 -0500 Subject: [PATCH 06/18] Using OS-specific error codes in `udsErrors()`. --- lib/constants.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/constants.js b/lib/constants.js index d6760a3..8abc476 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -1,3 +1,6 @@ +const os = require('os'), + process = require('process'); + exports.PROTOCOL = { TCP: 'tcp', UDS: 'uds', @@ -6,7 +9,22 @@ exports.PROTOCOL = { }; function udsErrors() { - return [107, 111]; + if (process.platform == 'linux') { + return [ + os.constants.errno.ENOTCONN, + os.constants.errno.ECONNREFUSED, + ] + } + + if (process.platform == 'darwin') { + return [ + os.constants.errno.EDESTADDRREQ, + os.constants.errno.ECONNRESET, + ] + } + + // Unknown / not yet implemented + return [] }; exports.udsErrors = udsErrors; From 46da650708ccf6126ce4641890cc478b77ac84df Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:45:04 -0500 Subject: [PATCH 07/18] Factoring out `udsErrorHandler()` helper. --- lib/statsd.js | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index 65592dd..16ac808 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -120,25 +120,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, const lastSocketCreateTime = Date.now(); this.socket.on('error', (err) => { if (UDS_ERROR_CODES.includes(-err.code)) { - if (Date.now() - lastSocketCreateTime >= this.udsGracefulRestartRateLimit) { - // recreate the socket, but only once per 30 seconds - if (this.errorHandler) { - this.socket.removeListener('error', this.errorHandler); - } - this.socket.close(); - this.socket = createTransport(this, { - host: this.host, - path: this.path, - port: this.port, - protocol: this.protocol - }); - - if (this.errorHandler) { - this.socket.on('error', this.errorHandler); - } else { - this.socket.on('error', error => console.error(`hot-shots UDS error: ${error}`)); - } - } + udsErrorHandler(this, lastSocketCreateTime); } }); } @@ -515,6 +497,30 @@ Client.prototype.childClient = function (options) { exports = module.exports = Client; exports.StatsD = Client; +function udsErrorHandler(client, lastSocketCreateTime) { + // recreate the socket, but only once within `udsGracefulRestartRateLimit`. + if (Date.now() - lastSocketCreateTime < client.udsGracefulRestartRateLimit) { + return; + } + + if (client.errorHandler) { + client.socket.removeListener("error", client.errorHandler); + } + client.socket.close(); + client.socket = createTransport(client, { + host: client.host, + path: client.path, + port: client.port, + protocol: client.protocol, + }); + + if (client.errorHandler) { + client.socket.on("error", client.errorHandler); + } else { + client.socket.on("error", (error) => console.error(`hot-shots UDS error: ${error}`)); + } +} + function trySetNewSocket(client) { client.socket = createTransport(client, { host: client.host, From 5d194ab607e82e623fd4849d68596fe641e79027 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:45:51 -0500 Subject: [PATCH 08/18] Factoring out `addUDSErrorHandler()` helper. --- lib/statsd.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index 16ac808..9910e96 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -118,11 +118,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, // does not support options.isChild (how to re-create a socket you didn't create?) if (this.socket && !options.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { const lastSocketCreateTime = Date.now(); - this.socket.on('error', (err) => { - if (UDS_ERROR_CODES.includes(-err.code)) { - udsErrorHandler(this, lastSocketCreateTime); - } - }); + addUDSErrorHandler(this, lastSocketCreateTime); } @@ -521,6 +517,14 @@ function udsErrorHandler(client, lastSocketCreateTime) { } } +function addUDSErrorHandler(client, lastSocketCreateTime) { + client.socket.on('error', (err) => { + if (UDS_ERROR_CODES.includes(-err.code)) { + udsErrorHandler(client, lastSocketCreateTime); + } + }); +} + function trySetNewSocket(client) { client.socket = createTransport(client, { host: client.host, From a8845b288c65b15813685517070abef87e6bd6af Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:47:18 -0500 Subject: [PATCH 09/18] Keeping around `client.isChild` so it can be re-used. --- lib/statsd.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 9910e96..706aafa 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -73,6 +73,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, this.errorHandler = options.errorHandler; this.udsGracefulErrorHandling = 'udsGracefulErrorHandling' in options ? options.udsGracefulErrorHandling : true; this.udsGracefulRestartRateLimit = options.udsGracefulRestartRateLimit || UDS_DEFAULT_GRACEFUL_RESTART_LIMIT; // only recreate once per second + this.isChild = options.isChild; // If we're mocking the client, create a buffer to record the outgoing calls. if (this.mock) { @@ -116,7 +117,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, // will gracefully (attempt) to re-open the socket with a small delay // options.udsGracefulRestartRateLimit is the minimum time (ms) between creating sockets // does not support options.isChild (how to re-create a socket you didn't create?) - if (this.socket && !options.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { + if (this.socket && !this.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { const lastSocketCreateTime = Date.now(); addUDSErrorHandler(this, lastSocketCreateTime); } From 23b6aaadbb87892fbe091f76be4a4b97ee628847 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:48:00 -0500 Subject: [PATCH 10/18] Removing spurious empty line. --- lib/statsd.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 706aafa..297e082 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -122,7 +122,6 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, addUDSErrorHandler(this, lastSocketCreateTime); } - this.messagesInFlight = 0; this.CHECKS = { OK: 0, From 572cb0c64ba9708d5fae8097cba6e3dcc6f01707 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:49:40 -0500 Subject: [PATCH 11/18] Adding UDS recovery to `Client.prototype.sendMessage`. --- lib/statsd.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 297e082..bf4ebc8 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -320,7 +320,16 @@ Client.prototype.sendMessage = function (message, callback) { return; } - if (!this.socket) { + const socketWasMissing = !this.socket; + if (socketWasMissing && this.protocol == PROTOCOL.UDS) { + trySetNewSocket(this); + if (this.socket && !this.isChild && this.udsGracefulErrorHandling) { + // On success, add custom UDS error handling. + addUDSErrorHandler(this, Date.now()); + } + } + + if (socketWasMissing) { const error = new Error('Socket not created properly. Check previous errors for details.'); if (callback) { return callback(error); From 0fc3a68e0be9b4cc4ae6f1403f876ed7a1a8561a Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:52:44 -0500 Subject: [PATCH 12/18] Checking `isChild` / `udsGracefulErrorHandling` in `maybeAddUDSErrorHandler()`. --- lib/statsd.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index bf4ebc8..95d8955 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -117,9 +117,9 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, // will gracefully (attempt) to re-open the socket with a small delay // options.udsGracefulRestartRateLimit is the minimum time (ms) between creating sockets // does not support options.isChild (how to re-create a socket you didn't create?) - if (this.socket && !this.isChild && options.protocol === PROTOCOL.UDS && this.udsGracefulErrorHandling) { + if (this.socket && options.protocol === PROTOCOL.UDS) { const lastSocketCreateTime = Date.now(); - addUDSErrorHandler(this, lastSocketCreateTime); + maybeAddUDSErrorHandler(this, lastSocketCreateTime); } this.messagesInFlight = 0; @@ -323,9 +323,9 @@ Client.prototype.sendMessage = function (message, callback) { const socketWasMissing = !this.socket; if (socketWasMissing && this.protocol == PROTOCOL.UDS) { trySetNewSocket(this); - if (this.socket && !this.isChild && this.udsGracefulErrorHandling) { + if (this.socket) { // On success, add custom UDS error handling. - addUDSErrorHandler(this, Date.now()); + maybeAddUDSErrorHandler(this, Date.now()); } } @@ -526,7 +526,11 @@ function udsErrorHandler(client, lastSocketCreateTime) { } } -function addUDSErrorHandler(client, lastSocketCreateTime) { +function maybeAddUDSErrorHandler(client, lastSocketCreateTime) { + if (client.isChild || !client.udsGracefulErrorHandling) { + return; + } + client.socket.on('error', (err) => { if (UDS_ERROR_CODES.includes(-err.code)) { udsErrorHandler(client, lastSocketCreateTime); From 7b36227f042e531c465a8528d7fa9d1e60708c57 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:55:53 -0500 Subject: [PATCH 13/18] Modifying `udsErrorHandler()` to check truthy socket. This way `this.socket` is only replaced if `createTransport()` returned an actual socket (vs. `null`). --- lib/statsd.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/statsd.js b/lib/statsd.js index 95d8955..24d3e35 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -511,13 +511,21 @@ function udsErrorHandler(client, lastSocketCreateTime) { if (client.errorHandler) { client.socket.removeListener("error", client.errorHandler); } - client.socket.close(); - client.socket = createTransport(client, { + + const newSocket = createTransport(client, { host: client.host, path: client.path, port: client.port, protocol: client.protocol, }); + if (newSocket) { + client.socket.close(); + client.socket = newSocket; + maybeAddUDSErrorHandler(this, Date.now()); + } else { + console.error('Could not replace UDS connection with new socket'); + return; + } if (client.errorHandler) { client.socket.on("error", client.errorHandler); From 7cf8ad821861a04cecb2f23d4dd977fb3dfa223d Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:57:24 -0500 Subject: [PATCH 14/18] Remove spurious space between `!` and var. --- lib/statsd.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 24d3e35..33d798e 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -100,7 +100,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock, } } - if (! this.socket) { + if (!this.socket) { trySetNewSocket(this); } From 1e364f50ae8c9af0c35156c56cb9422f4c9d306d Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 13:57:59 -0500 Subject: [PATCH 15/18] Adding explicit import for `process`. --- lib/statsd.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 33d798e..19f83d7 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -1,4 +1,5 @@ -const util = require('util'), +const process = require('process'), + util = require('util'), helpers = require('./helpers'), applyStatsFns = require('./statsFunctions'); From 15e9eb14aedb71a1199b567dd8741f5e8aeeb6b5 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 14:30:44 -0500 Subject: [PATCH 16/18] Adding JSDoc to newly added code. See: https://travis-ci.org/github/brightcove/hot-shots/jobs/730354851 This was due to `eslint` failures. Also changed some `==` to `===` and added / removed some `;` in lines that needed / didn't need. --- lib/constants.js | 17 +++++++++++------ lib/statsd.js | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/lib/constants.js b/lib/constants.js index 8abc476..7fb2bc7 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -8,23 +8,28 @@ exports.PROTOCOL = { STREAM: 'stream' }; +/** + * Determines error codes that signify a connection to a Unix Domain Socket (UDS) + * has failed in a way that can be retried. This codes are OS-specific. + * @returns {number[]} An array of the error codes. + */ function udsErrors() { - if (process.platform == 'linux') { + if (process.platform === 'linux') { return [ os.constants.errno.ENOTCONN, os.constants.errno.ECONNREFUSED, - ] + ]; } - if (process.platform == 'darwin') { + if (process.platform === 'darwin') { return [ os.constants.errno.EDESTADDRREQ, os.constants.errno.ECONNRESET, - ] + ]; } // Unknown / not yet implemented - return [] -}; + return []; +} exports.udsErrors = udsErrors; diff --git a/lib/statsd.js b/lib/statsd.js index 19f83d7..2e254bd 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -322,7 +322,7 @@ Client.prototype.sendMessage = function (message, callback) { } const socketWasMissing = !this.socket; - if (socketWasMissing && this.protocol == PROTOCOL.UDS) { + if (socketWasMissing && this.protocol === PROTOCOL.UDS) { trySetNewSocket(this); if (this.socket) { // On success, add custom UDS error handling. @@ -503,6 +503,19 @@ Client.prototype.childClient = function (options) { exports = module.exports = Client; exports.StatsD = Client; +/** + * Handle an error connecting to a Unix Domain Socket (UDS). This will + * attempt to create a new socket and replace and close the client's current + * socket, registering a **new** `udsErrorHandler()` on the newly created socket. + * If a new socket can't be created (e.g. if no UDS currently exists at + * `client.path`) then this will leave the existing socket intact. + * + * Note that this will no-op with an early exit if the last socket create time + * was too recent (within the UDS graceful restart rate limit). + * @param client Client The statsd Client that may be getting a UDS error handler. + * @param lastSocketCreateTime number The timestamp (in milliseconds since the + * epoch) when the current socket was created. + */ function udsErrorHandler(client, lastSocketCreateTime) { // recreate the socket, but only once within `udsGracefulRestartRateLimit`. if (Date.now() - lastSocketCreateTime < client.udsGracefulRestartRateLimit) { @@ -510,7 +523,7 @@ function udsErrorHandler(client, lastSocketCreateTime) { } if (client.errorHandler) { - client.socket.removeListener("error", client.errorHandler); + client.socket.removeListener('error', client.errorHandler); } const newSocket = createTransport(client, { @@ -529,12 +542,20 @@ function udsErrorHandler(client, lastSocketCreateTime) { } if (client.errorHandler) { - client.socket.on("error", client.errorHandler); + client.socket.on('error', client.errorHandler); } else { - client.socket.on("error", (error) => console.error(`hot-shots UDS error: ${error}`)); + client.socket.on('error', (error) => console.error(`hot-shots UDS error: ${error}`)); } } +/** + * Add a Unix Domain Socket (UDS) error handler to the client's socket, if the + * client is not a "child" client and has graceful error handling enabled for + * UDS. + * @param client Client The statsd Client that may be getting a UDS error handler. + * @param lastSocketCreateTime number The timestamp (in milliseconds since the + * epoch) when the current socket was created. + */ function maybeAddUDSErrorHandler(client, lastSocketCreateTime) { if (client.isChild || !client.udsGracefulErrorHandling) { return; @@ -547,6 +568,11 @@ function maybeAddUDSErrorHandler(client, lastSocketCreateTime) { }); } +/** + * Try to replace a client's socket with a new transport. If `createTransport()` + * returns `null` this will still set the client's socket to `null`. + * @param client Client The statsd Client that will be getting a new socket + */ function trySetNewSocket(client) { client.socket = createTransport(client, { host: client.host, From f3d3fd66bbf0de887b051e051a42c9b3a167b6ce Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 25 Sep 2020 15:04:33 -0500 Subject: [PATCH 17/18] Fixing `this` -> `client` reference. This "bug" was introduced to my bugfix in a re-factor. --- lib/statsd.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/statsd.js b/lib/statsd.js index 2e254bd..e3c0363 100644 --- a/lib/statsd.js +++ b/lib/statsd.js @@ -535,7 +535,7 @@ function udsErrorHandler(client, lastSocketCreateTime) { if (newSocket) { client.socket.close(); client.socket = newSocket; - maybeAddUDSErrorHandler(this, Date.now()); + maybeAddUDSErrorHandler(client, Date.now()); } else { console.error('Could not replace UDS connection with new socket'); return; From 884c9806579f70040e4e83618002bae33496cc59 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Mon, 28 Sep 2020 09:56:06 -0500 Subject: [PATCH 18/18] Changing unit test hardcoded references to 107/111. --- test/errorHandling.js | 62 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/test/errorHandling.js b/test/errorHandling.js index fb14000..fdef8a2 100644 --- a/test/errorHandling.js +++ b/test/errorHandling.js @@ -1,4 +1,6 @@ const assert = require('assert'); +const os = require('os'); +const process = require('process'); const helpers = require('./helpers/helpers.js'); const closeAll = helpers.closeAll; @@ -179,12 +181,11 @@ describe('#errorHandling', () => { }); if (serverType === 'uds' && clientType === 'client') { - it('should re-create the socket on 111 error for type uds', (done) => { - const code = 111; + it('should re-create the socket on bad connection error for type uds', (done) => { + const code = badUDSConnectionCode(); const realDateNow = Date.now; Date.now = () => '4857394578'; // emit an error, like a socket would - // 111 is connection refused server = createServer('uds_broken', opts => { const client = statsd = createHotShotsClient(Object.assign(opts, { protocol: 'uds', @@ -212,12 +213,11 @@ describe('#errorHandling', () => { }); }); - it('should re-create the socket on 107 error for type uds', (done) => { - const code = 107; + it('should re-create the socket on bad descriptor error for type uds', (done) => { + const code = badUDSDescriptorCode(); const realDateNow = Date.now; Date.now = () => '4857394578'; // emit an error, like a socket would - // 111 is connection refused server = createServer('uds_broken', opts => { const client = statsd = createHotShotsClient(Object.assign(opts, { protocol: 'uds', @@ -246,12 +246,11 @@ describe('#errorHandling', () => { }); it('should re-create the socket on error for type uds with the configurable limit', (done) => { - const code = 111; + const code = badUDSConnectionCode(); const limit = 4000; const realDateNow = Date.now; Date.now = () => '4857394578'; // emit an error, like a socket would - // 111 is connection refused server = createServer('uds_broken', opts => { const client = statsd = createHotShotsClient(Object.assign(opts, { protocol: 'uds', @@ -287,11 +286,10 @@ describe('#errorHandling', () => { }); it('should not re-create the socket on error for type uds with udsGracefulErrorHandling set to false', (done) => { - const code = 111; + const code = badUDSConnectionCode(); const realDateNow = Date.now; Date.now = () => '4857394578'; // emit an error, like a socket would - // 111 is connection refused server = createServer('uds_broken', opts => { const client = statsd = createHotShotsClient(Object.assign(opts, { protocol: 'uds', @@ -323,3 +321,47 @@ describe('#errorHandling', () => { }); }); }); + +/** + * Return system error code for a "bad connection" to a UDS (e.g. does not + * exist). + * + * The value is negated because of the way errors are returned, e.g. by `libuv`. + * + * - 111 (ECONNREFUSED) on Linux + * - 54 (ECONNRESET) on macOS + * - "not-implemented" on other platforms + */ +function badUDSConnectionCode() { + if (process.platform === 'linux') { + return -os.constants.errno.ECONNREFUSED; + } + + if (process.platform === 'darwin') { + return -os.constants.errno.ECONNRESET; + } + + return 'not-implemented'; +} + +/** + * Return system error code for a "bad descriptor" (e.g. descriptor exists + * but server is gone). + * + * The value is negated because of the way errors are returned, e.g. by `libuv`. + * + * - 107 (ENOTCONN) on Linux + * - 39 (EDESTADDRREQ) on macOS + * - "not-implemented" on other platforms + */ +function badUDSDescriptorCode() { + if (process.platform === 'linux') { + return -os.constants.errno.ENOTCONN; + } + + if (process.platform === 'darwin') { + return -os.constants.errno.EDESTADDRREQ; + } + + return 'not-implemented'; +}