From ac5ec2193a10223d3f875c402a14aa6b49a2da16 Mon Sep 17 00:00:00 2001 From: Joe Krill Date: Thu, 25 Oct 2018 08:27:04 -0400 Subject: [PATCH] fix: destroy socket on connection failure/timeout --- src/ElkClient.test.ts | 79 ++++++++++++++++++---- src/ElkClient.ts | 50 ++++++++------ src/connection/ElkSocketConnection.test.ts | 10 +-- src/connection/ElkSocketConnection.ts | 4 +- 4 files changed, 101 insertions(+), 42 deletions(-) diff --git a/src/ElkClient.test.ts b/src/ElkClient.test.ts index 03848413..db314241 100644 --- a/src/ElkClient.test.ts +++ b/src/ElkClient.test.ts @@ -157,25 +157,23 @@ describe('ElkClient', () => { describe('no credentials needed', () => { test('becomes ready', async () => { - expect.assertions(2); - setTimeout(() => { - // No requests is ever made for username/password, but the panel - // emits the ethernet test response. - client.connection.emit('data', '16XK2636115020605110006F\r\n'); + let connectedEmitted = false; + let readyEmitted = false; + expect.assertions(4); + client.once('connected', () => { + connectedEmitted = true; + }); + client.once('ready', () => { + readyEmitted = true; }); - await client.connect(); - expect(client.state).toBe(ElkClientState.Ready); - expect(client.authenticated).toBe(false); - }); - - test('requests the version number when connected', async () => { - expect.assertions(1); setTimeout(() => { client.connection.emit('connected'); - client.connection.emit('data', 'OK\r\n'); }); await client.connect(); - expect(client.connection.write).toHaveBeenCalledWith('06vn0056\r\n'); + expect(connectedEmitted).toBe(true); + expect(readyEmitted).toBe(true); + expect(client.state).toBe(ElkClientState.Ready); + expect(client.authenticated).toBe(false); }); }); @@ -330,6 +328,59 @@ describe('ElkClient', () => { } }); }); + + describe('when not needed', () => { + let readyEmitted = false; + + beforeEach(() => { + client.once('ready', () => { + readyEmitted = true; + }); + }); + + test('emits "ready" when data is received', async () => { + setTimeout(() => { + client.connection.emit('data', '16XK2516135251018110006C\r\n'); + }); + await client.connect(); + expect(readyEmitted).toBe(true); + expect(client.isReady).toBe(true); + }); + }); + + describe('while authenticating', () => { + let connectPromise: Promise; + let messageEmitCount: number = 0; + let okEmitCount: number = 0; + + beforeEach(() => { + connectPromise = client.connect(5); + mockSocketConnectionInstance.write.mockClear(); + client.connection.emit('data', '\r\nUsername: '); + client.on('message', () => { + messageEmitCount++; + }); + client.on('message', () => { + okEmitCount++; + }); + }); + + afterEach(async () => { + // Let the promise reject. + client.removeAllListeners(); + return connectPromise.catch(() => undefined); + }); + + test('ignores non-auth related messages ', async () => { + client.connection.emit('data', '\r\nUsername: '); + expect(client.state).toBe(ElkClientState.Authenticating); + client.connection.emit('data', 'OK\r\n'); + client.connection.emit('data', '16XK2636115020605110006F\r\n'); + client.connection.emit('data', 'OK\r\n'); + expect(messageEmitCount).toBe(0); + expect(okEmitCount).toBe(0); + }); + }); }); }); diff --git a/src/ElkClient.ts b/src/ElkClient.ts index a41c8c25..be0df23a 100644 --- a/src/ElkClient.ts +++ b/src/ElkClient.ts @@ -4,7 +4,7 @@ import ElkConnectionState from './connection/ElkConnectionState'; import ElkClientCommands from './ElkClientCommands'; import ElkClientState from './ElkClientState'; import AuthenticationFailedError, { - AuthenticationFailedReason + AuthenticationFailedReason, } from './errors/AuthenticationFailedError'; import ElkSocketConnection from './connection/ElkSocketConnection'; import ElkClientOptions from './ElkClientOptions'; @@ -153,7 +153,7 @@ class ElkClient extends ElkClientCommands { * when completed. */ async disconnect(): Promise { - return this._connection.disconnect().then(() => undefined); + await this._connection.disconnect(); } /** @@ -172,16 +172,23 @@ class ElkClient extends ElkClientCommands { this.emit('ready'); } - private onConnectionConnected = () => { + private onConnectionConnected = async () => { + this.emit('connected'); + + // At this point we may need to authenticate. + // The panel will send "\r\nUsername: " to request + // the username. and that will cause the authentication + // process to be triggered and that will eventually + // lead to a "ready" or "error" being emitted, depending + // on whether the authentication was successful. + // But in the case that we DON'T need to authenticate, + // we just immediately trigger the ready state. Otherwise + // we'd have to wait for the panel to emit some data + // to trigger that, and that could be up to 30 seconds + // depending on the timing (an "XK" message is emitted + // every 30 seconds). if (!this.options.username) { - // If no user name was supplied then send a command - // to test the state of our connection. If we don't - // need authentication, the onConnectionData callback - // will see the response and mark the state as ready. - // If we do need to authenticate, this should cause - // the control panel to issue a failure message or request - // authentication. - void this.getVersionNumber(); + this.onReady(false); } }; @@ -200,7 +207,7 @@ class ElkClient extends ElkClientCommands { this.emit('error', error); }; - private onConnectionData = (data: string) => { + private onConnectionData = async (data: string) => { // TODO: Does each `data` chunk always contain complete // messages? Do we need to consider that we might get // a partial message? I've seen _multiple_ complete @@ -226,7 +233,7 @@ class ElkClient extends ElkClientCommands { 'Username was requested but none was provided.' ) ); - this.disconnect().catch(() => undefined); + await this.disconnect(); return; } @@ -244,7 +251,7 @@ class ElkClient extends ElkClientCommands { 'Password was requested but none was provided.' ) ); - this.disconnect().catch(() => undefined); + await this.disconnect(); return; } void this._connection.write(this.options.password + '\r\n'); @@ -258,7 +265,7 @@ class ElkClient extends ElkClientCommands { 'Login failed, invalid username or password.' ) ); - this.disconnect().catch(() => undefined); + await this.disconnect(); return; } case LOGIN_SUCCESSFUL: { @@ -270,20 +277,25 @@ class ElkClient extends ElkClientCommands { // because the username and password will be echoed back if (this._state !== ElkClientState.Authenticating) { if (!this.isReady && this.isConnected) { + // If we're getting data and connected, but not "ready", + // we need to switch to a ready state. This could happen if + // a username was provided but not requested by the control + // panel. this.onReady(); } // The M1 doesn't always send a single packet, so we need to check // for multiple packets at once. - data.split(/\r\n|\r|\n/).forEach(packet => { - if (packet) { + data + .split(/\r\n|\r|\n/) + .filter(packet => !!packet) + .forEach(packet => { if (packet === 'OK') { this.emit('ok'); } else { this.emit('message', parseElkResponse(packet + '\r\n')); } - } - }); + }); } } } diff --git a/src/connection/ElkSocketConnection.test.ts b/src/connection/ElkSocketConnection.test.ts index e9128534..e70c876d 100644 --- a/src/connection/ElkSocketConnection.test.ts +++ b/src/connection/ElkSocketConnection.test.ts @@ -310,13 +310,7 @@ describe('ElkSocketConnection', () => { describe('times out', () => { beforeEach(() => { - mockCreateSocketConnectsSuccessfully(); - mocked(createSocket).mockImplementation(() => { - const mock = new SocketMock(); - mock.setConnecting(); - setTimeout(() => mock.setConnected(), 10); - return mock; - }); + mockCreateSocketConnectsSuccessfully(10); connection = new ElkSocketConnection(); }); @@ -491,7 +485,7 @@ describe('ElkSocketConnection', () => { }); afterEach(async () => { - connectPromise.catch(() => undefined); + return connectPromise.catch(() => undefined); }); it('cancels the connect and resolves', async () => { diff --git a/src/connection/ElkSocketConnection.ts b/src/connection/ElkSocketConnection.ts index 02467f81..a181e6fb 100644 --- a/src/connection/ElkSocketConnection.ts +++ b/src/connection/ElkSocketConnection.ts @@ -216,6 +216,7 @@ class ElkSocketConnection extends EventEmitter implements ElkConnection { }) ) .catch(error => { + socket.destroy(); socket.removeListener('connect', connectListener); this.removeListener('error', errorListener); this.removeListener('disconnecting', disconnectingListener); @@ -240,11 +241,12 @@ class ElkSocketConnection extends EventEmitter implements ElkConnection { return withTimeout( timeout, - new Promise((resolve, reject) => { + new Promise(resolve => { if (!this._socket) { // Already disconnected, just resolve. return resolve(this); } + socket = this._socket; if (this.state === ElkConnectionState.Connecting) {