From 0fc9c72358802a673b925cd9084a85c0717f662b Mon Sep 17 00:00:00 2001 From: cornholio <0@mcornholio.ru> Date: Sun, 4 Aug 2019 18:40:18 +0300 Subject: [PATCH] Replaced ping-based healthcheck with net.createConnection Older ping-based healthcheck had a bunch of problems: * Doesn't work in ipv6-only environment (because of calling `ping`, not `ping6`) * Doesn't work if ICMP is blocked, but TCP is not * Doesn't work as expected if TCP on this exact port is firewalled out * Uses a lot of CPU on process spawning, may cause DoS in clusters with large number of worker processes Healthcheck with net.createConnection seems to be a much simpler and more robust alternative Two test were needed changing: * One was implicitly tied to ping interval; ECONNREFUSED was thrown faster, I've tuned reconnect timeout in test * Another one actually relied upon one of aforementioned problems: setting up server at 127.0.0.1:1234 would mean server is available to ping, but not to TCP connection. In that test, I've used net.listen to actually listen for connections on socket --- lib/connection.js | 28 +++++--------- test/memcached-connections.test.js | 61 ++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 39 deletions(-) diff --git a/lib/connection.js b/lib/connection.js index c055944..bf15ddd 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -1,31 +1,23 @@ "use strict"; var EventEmitter = require('events').EventEmitter - , spawn = require('child_process').spawn + , net = require('net') , Utils = require('./utils') , util = require('util'); exports.IssueLog = IssueLog; // connection issue handling -exports.Available = ping; // connection availablity -function ping (host, callback) { - var isWin = process.platform.indexOf('win') === 0; // win32 or win64 - var arg = isWin ? '-n' : '-c'; - var pong = spawn('ping', [arg, '3', host]); // only ping 3 times - - pong.on('error', function onerror (error) { - callback(error, false); - pong.kill(); +function ping (host, port, timeout, callback) { + var socket = net.createConnection({ port: port, host: host }, function (err) { + callback(err); + socket.end(); }); - pong.stdout.on('data', function stdoutdata (data) { - callback(false, data.toString().split('\n')[0].substr(14)); - pong.kill(); - }); + socket.setTimeout(timeout); - pong.stderr.on('data', function stderrdata (data) { - callback(new Error(data.toString().split('\n')[0].substr(14)), false); - pong.kill(); + socket.on('error', function (err) { + callback(err); + socket.end(); }); } @@ -115,7 +107,7 @@ issues.attemptReconnect = function attemptReconnect () { this.emit('reconnecting', this.details); // Ping the server - ping(this.tokens[1], function pingpong (err) { + ping(this.tokens[1], this.tokens[0] || 11211, this.config.maxTimeout || 100, function pingpong (err) { // still no access to the server if (err) { issue.messages.push(err.message || 'No message specified'); diff --git a/test/memcached-connections.test.js b/test/memcached-connections.test.js index cadea3c..53ed321 100644 --- a/test/memcached-connections.test.js +++ b/test/memcached-connections.test.js @@ -8,6 +8,7 @@ var assert = require('assert') , fs = require('fs') + , net = require('net') , common = require('./common') , Memcached = require('../'); @@ -122,7 +123,9 @@ describe('Memcached connections', function () { minTimeout: 0, maxTimeout: 100, failures: 0, - reconnect: 100 }) + // Reconnect timeout of 300ms is tied to 400ms test timeout, + // so only one reconnection attempt could occur + reconnect: 300 }) , reconnectAttempts = 0; memcached.on('reconnecting', function() { @@ -145,7 +148,7 @@ describe('Memcached connections', function () { }); }); it('should reset failures after reconnecting to failed server', function(done) { - var server = '127.0.0.1:1234' + var server = '127.0.0.1:12340' , memcached = new Memcached(server, { retries: 0, minTimeout: 0, @@ -155,32 +158,48 @@ describe('Memcached connections', function () { reconnect: 100 }) this.timeout(60000); - // First request will mark server failed memcached.get('idontcare', function(err) { assert.throws(function() { throw err }, /connect ECONNREFUSED/); - // Wait 10ms, server should be back online - setTimeout(function() { - // Second request will mark server dead - memcached.get('idontcare', function(err) { - assert.throws(function() { throw err }, /connect ECONNREFUSED/); - // Third request should find no servers + + // Setting server up + var fakeServer = net.createServer(); + fakeServer.listen(12340, '127.0.0.1', function() { + // Wait 10ms, server should be back online + setTimeout(function() { + // Tearing server down + fakeServer.close(function () { + // Second request will mark server dead memcached.get('idontcare', function(err) { - assert.throws(function() { throw err }, /not available/); - // Give enough time for server to reconnect - setTimeout(function() { - // Server should be reconnected, but expect ECONNREFUSED - memcached.get('idontcare', function(err) { - assert.throws(function() { throw err }, /connect ECONNREFUSED/); - assert.deepEqual(memcached.issues[server].failures, - memcached.issues[server].config.failures); - memcached.end(); - done(); + assert.throws(function() { throw err }, /connect ECONNREFUSED/); + // Third request should find no servers + memcached.get('idontcare', function(err) { + assert.throws(function() { throw err }, /not available/); + + // Setting server up once again + fakeServer = net.createServer(); + fakeServer.listen(12340, '127.0.0.1', function() { + // Give enough time for server to reconnect + setTimeout(function() { + // And tearing new server down too + fakeServer.close(function () { + // Server should be reconnected, but expect ECONNREFUSED + memcached.get('idontcare', function(err) { + assert.throws(function() { throw err }, /connect ECONNREFUSED/); + + assert.deepEqual(memcached.issues[server].failures, + memcached.issues[server].config.failures); + memcached.end(); + done(); + }); + }); + }, 150); }); - }, 150); + }); }); }); - },10); + },10); + }); }); }); it('should default to port 11211', function(done) {