From d9fca62c17c10f24a1fa15d5130e05e61267a44e Mon Sep 17 00:00:00 2001 From: Roman Shtylman Date: Wed, 16 May 2018 17:07:58 -0400 Subject: [PATCH] grace period disconnect should be per-client --- lib/Client.js | 45 ++++++++++++++++++++++++++++++++++++--- lib/Client.test.js | 1 - lib/ClientManager.js | 35 +++++++++--------------------- lib/ClientManager.test.js | 33 ++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/lib/Client.js b/lib/Client.js index 16b522e..369eb07 100644 --- a/lib/Client.js +++ b/lib/Client.js @@ -1,21 +1,60 @@ import http from 'http'; import Debug from 'debug'; import pump from 'pump'; +import EventEmitter from 'events'; // A client encapsulates req/res handling using an agent // // If an agent is destroyed, the request handling will error // The caller is responsible for handling a failed request -class Client { +class Client extends EventEmitter { constructor(options) { - this.agent = options.agent; - this.debug = Debug('lt:Client'); + super(); + + const agent = this.agent = options.agent; + const id = this.id = options.id; + + this.debug = Debug(`lt:Client[${this.id}]`); + + // client is given a grace period in which they can connect before they are _removed_ + this.graceTimeout = setTimeout(() => { + this.close(); + }, 1000).unref(); + + agent.on('online', () => { + this.debug('client online %s', id); + clearTimeout(this.graceTimeout); + }); + + agent.on('offline', () => { + this.debug('client offline %s', id); + + // if there was a previous timeout set, we don't want to double trigger + clearTimeout(this.graceTimeout); + + // client is given a grace period in which they can re-connect before they are _removed_ + this.graceTimeout = setTimeout(() => { + this.close(); + }, 1000).unref(); + }); + + // TODO(roman): an agent error removes the client, the user needs to re-connect? + // how does a user realize they need to re-connect vs some random client being assigned same port? + agent.once('error', (err) => { + this.close(); + }); } stats() { return this.agent.stats(); } + close() { + clearTimeout(this.graceTimeout); + this.agent.destroy(); + this.emit('close'); + } + handleRequest(req, res) { this.debug('> %s', req.url); const opt = { diff --git a/lib/Client.test.js b/lib/Client.test.js index 8f53be1..5a0afd0 100644 --- a/lib/Client.test.js +++ b/lib/Client.test.js @@ -1,7 +1,6 @@ import assert from 'assert'; import http from 'http'; import { Duplex } from 'stream'; -import EventEmitter from 'events'; import WebSocket from 'ws'; import net from 'net'; diff --git a/lib/ClientManager.js b/lib/ClientManager.js index 6279982..e1a7838 100644 --- a/lib/ClientManager.js +++ b/lib/ClientManager.js @@ -21,6 +21,7 @@ class ClientManager { this.debug = Debug('lt:ClientManager'); + // This is totally wrong :facepalm: this needs to be per-client... this.graceTimeout = null; } @@ -42,35 +43,19 @@ class ClientManager { maxSockets: 10, }); - agent.on('online', () => { - this.debug('client online %s', id); - clearTimeout(this.graceTimeout); + const client = new Client({ + id, + agent, }); - agent.on('offline', () => { - // TODO(roman): grace period for re-connecting - // this period is short as the client is expected to maintain connections actively - // if they client does not reconnect on a dropped connection they need to re-establish - this.debug('client offline %s', id); - - // client is given a grace period in which they can re-connect before they are _removed_ - this.graceTimeout = setTimeout(() => { - this.removeClient(id); - }, 1000); - }); - - // TODO(roman): an agent error removes the client, the user needs to re-connect? - // how does a user realize they need to re-connect vs some random client being assigned same port? - agent.once('error', (err) => { - this.removeClient(id); - }); - - const client = new Client({ agent }); - // add to clients map immediately // avoiding races with other clients requesting same id clients[id] = client; + client.once('close', () => { + this.removeClient(id); + }); + // try/catch used here to remove client id try { const info = await agent.listen(); @@ -96,11 +81,11 @@ class ClientManager { } --this.stats.tunnels; delete this.clients[id]; - client.agent.destroy(); + client.close(); } hasClient(id) { - return this.clients[id]; + return !!this.clients[id]; } getClient(id) { diff --git a/lib/ClientManager.test.js b/lib/ClientManager.test.js index 3282553..d85abe5 100644 --- a/lib/ClientManager.test.js +++ b/lib/ClientManager.test.js @@ -54,4 +54,37 @@ describe('ClientManager', () => { await new Promise(resolve => setTimeout(resolve, 1500)); assert(!manager.hasClient('foobar')); }).timeout(5000); + + it('should remove correct client once it goes offline', async () => { + const manager = new ClientManager(); + const clientFoo = await manager.newClient('foo'); + const clientBar = await manager.newClient('bar'); + + const socket = await new Promise((resolve) => { + const netClient = net.createConnection({ port: clientFoo.port }, () => { + resolve(netClient); + }); + }); + + await new Promise(resolve => setTimeout(resolve, 1500)); + + // foo should still be ok + assert(manager.hasClient('foo')); + + // clientBar shound be removed - nothing connected to it + assert(!manager.hasClient('bar')); + + manager.removeClient('foo'); + socket.end(); + }).timeout(5000); + + it('should remove clients if they do not connect within 5 seconds', async () => { + const manager = new ClientManager(); + const clientFoo = await manager.newClient('foo'); + assert(manager.hasClient('foo')); + + // wait past grace period (1s) + await new Promise(resolve => setTimeout(resolve, 1500)); + assert(!manager.hasClient('foo')); + }).timeout(5000); });