From 7dc2f13ffabfd1f3e6b7c549629512a0b67ad33d Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 30 Jun 2017 16:43:43 +0100 Subject: [PATCH 01/14] fix(NA): send connection terminate only when we have a forceClose. --- src/client.ts | 6 +++++- src/test/tests.ts | 48 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/client.ts b/src/client.ts index 697956621..0a7f4ee15 100644 --- a/src/client.ts +++ b/src/client.ts @@ -125,7 +125,11 @@ export class SubscriptionClient { public close(isForced = true) { if (this.client !== null) { this.forceClose = isForced; - this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); + + if (this.forceClose) { + this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); + } + this.client.close(); this.client = null; } diff --git a/src/test/tests.ts b/src/test/tests.ts index 4f273e864..1c635659c 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -228,7 +228,7 @@ new SubscriptionServer(onConnectErrorOptions, { server: httpServerWithOnConnectE const httpServerWithDelay = createServer(notFoundRequestListener); httpServerWithDelay.listen(DELAYED_TEST_PORT); new SubscriptionServer(Object.assign({}, options, { - onSubscribe: (msg: OperationMessagePayload | any, params: SubscriptionOptions) => { + onSubscribe: (msg: OperationMessagePayload | any, params: SubscriptionOptions): any => { return new Promise((resolve, reject) => { setTimeout(() => { resolve(Object.assign({}, params, { context: msg['context'] })); @@ -1138,12 +1138,13 @@ describe('Client', function () { }, 200); }); - it('should force close the connection without tryReconnect', function (done) { + it.only('should force close the connection without tryReconnect', function (done) { const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { reconnect: true, reconnectionAttempts: 1, }); const tryReconnectSpy = sinon.spy(subscriptionsClient, 'tryReconnect'); + let receivedConnecitonTerminate = false; wsServer.on('connection', (connection: any) => { connection.on('message', (message: any) => { const parsedMessage = JSON.parse(message); @@ -1151,6 +1152,10 @@ describe('Client', function () { if (parsedMessage.type === MessageTypes.GQL_CONNECTION_INIT) { connection.send(JSON.stringify({ type: MessageTypes.GQL_CONNECTION_ACK, payload: {} })); } + + if (parsedMessage.type === MessageTypes.GQL_CONNECTION_TERMINATE) { + receivedConnecitonTerminate = true; + } }); }); @@ -1164,11 +1169,50 @@ describe('Client', function () { }; setTimeout(() => { + expect(receivedConnecitonTerminate).to.be.equal(true); expect(tryReconnectSpy.callCount).to.be.equal(0); expect(subscriptionsClient.status).to.be.equal(WebSocket.CLOSED); done(); }, 500); }); + + it.only('should close the connection without sent connection terminate and reconnect', function (done) { + const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { + reconnect: true, + reconnectionAttempts: 1, + }); + const tryReconnectSpy = sinon.spy(subscriptionsClient, 'tryReconnect'); + let receivedConnecitonTerminate = false; + wsServer.on('connection', (connection: any) => { + connection.on('message', (message: any) => { + const parsedMessage = JSON.parse(message); + // mock server + if (parsedMessage.type === MessageTypes.GQL_CONNECTION_INIT) { + connection.send(JSON.stringify({ type: MessageTypes.GQL_CONNECTION_ACK, payload: {} })); + } + + if (parsedMessage.type === MessageTypes.GQL_CONNECTION_TERMINATE) { + receivedConnecitonTerminate = true; + } + }); + }); + + const originalOnMessage = subscriptionsClient.client.onmessage; + subscriptionsClient.client.onmessage = (dataReceived: any) => { + let receivedDataParsed = JSON.parse(dataReceived.data); + if (receivedDataParsed.type === MessageTypes.GQL_CONNECTION_ACK) { + originalOnMessage(dataReceived); + subscriptionsClient.close(false); + } + }; + + setTimeout(() => { + expect(tryReconnectSpy.callCount).to.be.equal(1); + expect(subscriptionsClient.status).to.be.equal(WebSocket.OPEN); + expect(receivedConnecitonTerminate).to.be.equal(false); + done(); + }, 500); + }); }); describe('Server', function () { From 76de55309293a576bb30aa7944506633ed478976 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 30 Jun 2017 16:45:19 +0100 Subject: [PATCH 02/14] test(NA): removed only from tests. --- src/test/tests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/tests.ts b/src/test/tests.ts index 1c635659c..68c5d6148 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -1138,7 +1138,7 @@ describe('Client', function () { }, 200); }); - it.only('should force close the connection without tryReconnect', function (done) { + it('should force close the connection without tryReconnect', function (done) { const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { reconnect: true, reconnectionAttempts: 1, @@ -1176,7 +1176,7 @@ describe('Client', function () { }, 500); }); - it.only('should close the connection without sent connection terminate and reconnect', function (done) { + it('should close the connection without sent connection terminate and reconnect', function (done) { const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { reconnect: true, reconnectionAttempts: 1, From 52801786869f5b120009eefb0ab0fb50bce6579a Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 00:47:40 +0100 Subject: [PATCH 03/14] fix(NA): correctly handle timeout and ka. --- src/client.ts | 1 + src/test/tests.ts | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/client.ts b/src/client.ts index 0a7f4ee15..ad4e0769c 100644 --- a/src/client.ts +++ b/src/client.ts @@ -528,6 +528,7 @@ export class SubscriptionClient { if (this.checkConnectionTimeoutId) { clearTimeout(this.checkConnectionTimeoutId); + this.checkConnection(); } this.checkConnectionTimeoutId = setTimeout(this.checkConnection.bind(this), this.wsTimeout); break; diff --git a/src/test/tests.ts b/src/test/tests.ts index 68c5d6148..444d29298 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -215,7 +215,7 @@ new SubscriptionServer(options, { server: httpServer }); const httpServerWithKA = createServer(notFoundRequestListener); httpServerWithKA.listen(KEEP_ALIVE_TEST_PORT); -new SubscriptionServer(Object.assign({}, options, { keepAlive: 10 }), { server: httpServerWithKA }); +new SubscriptionServer(Object.assign({}, options, { keepAlive: 500 }), { server: httpServerWithKA }); const httpServerWithEvents = createServer(notFoundRequestListener); httpServerWithEvents.listen(EVENTS_TEST_PORT); @@ -1030,7 +1030,7 @@ describe('Client', function () { it('should take care of received keep alive', (done) => { let wasKAReceived = false; - const subscriptionsClient = new SubscriptionClient(`ws://localhost:${KEEP_ALIVE_TEST_PORT}/`, { timeout: 5 }); + const subscriptionsClient = new SubscriptionClient(`ws://localhost:${KEEP_ALIVE_TEST_PORT}/`, { timeout: 600 }); const originalOnMessage = subscriptionsClient.client.onmessage; subscriptionsClient.client.onmessage = (dataReceived: any) => { let receivedDataParsed = JSON.parse(dataReceived.data); @@ -1046,18 +1046,28 @@ describe('Client', function () { expect(wasKAReceived).to.equal(true); expect(subscriptionsClient.status).to.equal(WebSocket.CLOSED); done(); - }, 100); + }, 1200); }); it('should correctly clear timeout if receives ka too early', (done) => { - const subscriptionsClient = new SubscriptionClient(`ws://localhost:${KEEP_ALIVE_TEST_PORT}/`, { timeout: 25 }); + let receivedKeepAlive = 0; + + const subscriptionsClient = new SubscriptionClient(`ws://localhost:${KEEP_ALIVE_TEST_PORT}/`, { timeout: 600 }); const checkConnectionSpy = sinon.spy(subscriptionsClient, 'checkConnection'); + const originalOnMessage = subscriptionsClient.client.onmessage; + subscriptionsClient.client.onmessage = (dataReceived: any) => { + let receivedDataParsed = JSON.parse(dataReceived.data); + if (receivedDataParsed.type === MessageTypes.GQL_CONNECTION_KEEP_ALIVE) { + ++receivedKeepAlive; + originalOnMessage(dataReceived); + } + }; setTimeout(() => { - expect(checkConnectionSpy.callCount).to.be.equal(1); + expect(checkConnectionSpy.callCount).to.be.equal(receivedKeepAlive); expect(subscriptionsClient.status).to.be.equal(subscriptionsClient.client.OPEN); done(); - }, 100); + }, 1300); }); it('should take care of invalid message received', (done) => { From 945b1f94adf55a3afd1d0a6da7f8c8b837273936 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 03:14:19 +0100 Subject: [PATCH 04/14] fix(NA): connection close flow. --- src/client.ts | 28 ++++++++++++++++------------ src/test/tests.ts | 12 ++++++------ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/client.ts b/src/client.ts index ad4e0769c..4f9f35371 100644 --- a/src/client.ts +++ b/src/client.ts @@ -70,7 +70,7 @@ export class SubscriptionClient { private connectionCallback: any; private eventEmitter: EventEmitter; private lazy: boolean; - private forceClose: boolean; + private closedByUser: boolean; private wsImpl: any; private wasKeepAliveReceived: boolean; private checkConnectionTimeoutId: any; @@ -103,7 +103,7 @@ export class SubscriptionClient { this.reconnecting = false; this.reconnectionAttempts = reconnectionAttempts; this.lazy = !!lazy; - this.forceClose = false; + this.closedByUser = false; this.backoff = new Backoff({ jitter: 0.5 }); this.eventEmitter = new EventEmitter(); this.middlewares = []; @@ -122,16 +122,23 @@ export class SubscriptionClient { return this.client.readyState; } - public close(isForced = true) { + public close(isForced = true, closedByUser = true) { if (this.client !== null) { - this.forceClose = isForced; + this.closedByUser = closedByUser; - if (this.forceClose) { + if (isForced) { this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); } - this.client.close(); + if (closedByUser) { + this.client.close(); + } this.client = null; + this.eventEmitter.emit('disconnected'); + + if (!isForced) { + this.tryReconnect(); + } } } @@ -435,6 +442,7 @@ export class SubscriptionClient { this.client = new this.wsImpl(this.url, GRAPHQL_WS); this.client.onopen = () => { + this.closedByUser = false; this.eventEmitter.emit(this.reconnecting ? 'reconnecting' : 'connecting'); const payload: ConnectionParams = typeof this.connectionParams === 'function' ? this.connectionParams() : this.connectionParams; @@ -445,12 +453,8 @@ export class SubscriptionClient { }; this.client.onclose = () => { - this.eventEmitter.emit('disconnected'); - - if (this.forceClose) { - this.forceClose = false; - } else { - this.tryReconnect(); + if ( !this.closedByUser ) { + this.close(false, false); } }; diff --git a/src/test/tests.ts b/src/test/tests.ts index 444d29298..1da13e62e 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -314,7 +314,7 @@ describe('Client', function () { it('should emit disconnect event for client side when socket closed', (done) => { const client = new SubscriptionClient(`ws://localhost:${TEST_PORT}/`, { connectionCallback: () => { - client.client.close(); + client.client.close(1001); }, }); @@ -605,7 +605,7 @@ describe('Client', function () { connection.close(); setTimeout(() => { - expect(client.client.readyState).to.equals(WebSocket.CLOSED); + expect(client.status).to.equals(WebSocket.CLOSED); done(); }, 500); }); @@ -970,7 +970,7 @@ describe('Client', function () { }); const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { - timeout: 100, + timeout: 10, reconnect: true, reconnectionAttempts: 1, }); @@ -979,7 +979,7 @@ describe('Client', function () { setTimeout(() => { expect(connectSpy.callCount).to.be.equal(2); done(); - }, 500); + }, 1500); }); it('should stop trying to reconnect if not receives the ack from the server', function (done) { @@ -1174,7 +1174,7 @@ describe('Client', function () { let receivedDataParsed = JSON.parse(dataReceived.data); if (receivedDataParsed.type === MessageTypes.GQL_CONNECTION_ACK) { originalOnMessage(dataReceived); - subscriptionsClient.close(); + subscriptionsClient.close(true); } }; @@ -2013,7 +2013,7 @@ describe('Client<->Server Flow', () => { setTimeout(() => { // Disconnect the client - client.close(); + client.close(false); // Subscribe to data, without manually reconnect before const opId = client.subscribe({ From 6c0c1056006dc62162e8d30dcb4942a6db2bb042 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 03:21:39 +0100 Subject: [PATCH 05/14] test(NA): reuse some old test values. --- src/test/tests.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/tests.ts b/src/test/tests.ts index 1da13e62e..b92088370 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -314,7 +314,7 @@ describe('Client', function () { it('should emit disconnect event for client side when socket closed', (done) => { const client = new SubscriptionClient(`ws://localhost:${TEST_PORT}/`, { connectionCallback: () => { - client.client.close(1001); + client.client.close(); }, }); @@ -970,7 +970,7 @@ describe('Client', function () { }); const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { - timeout: 10, + timeout: 100, reconnect: true, reconnectionAttempts: 1, }); @@ -979,7 +979,7 @@ describe('Client', function () { setTimeout(() => { expect(connectSpy.callCount).to.be.equal(2); done(); - }, 1500); + }, 500); }); it('should stop trying to reconnect if not receives the ack from the server', function (done) { @@ -1174,7 +1174,7 @@ describe('Client', function () { let receivedDataParsed = JSON.parse(dataReceived.data); if (receivedDataParsed.type === MessageTypes.GQL_CONNECTION_ACK) { originalOnMessage(dataReceived); - subscriptionsClient.close(true); + subscriptionsClient.close(); } }; From 3dbf37bd8f6f334c26d209a5a3caac6a39fac7f4 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 04:06:52 +0100 Subject: [PATCH 06/14] feat(NA): reconnect on error. --- src/client.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/client.ts b/src/client.ts index 4f9f35371..e5a718b63 100644 --- a/src/client.ts +++ b/src/client.ts @@ -461,6 +461,7 @@ export class SubscriptionClient { this.client.onerror = () => { // Capture and ignore errors to prevent unhandled exceptions, wait for // onclose to fire before attempting a reconnect. + this.close(false, true); }; this.client.onmessage = ({ data }: {data: any}) => { From c5c94427fde618a487c85916c709619340cf588f Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 06:21:35 +0100 Subject: [PATCH 07/14] fix(NA): attemps to reconnect. --- src/client.ts | 34 ++++++++++++++++++++++++++-------- src/test/tests.ts | 30 +++++++++++++++--------------- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/client.ts b/src/client.ts index e5a718b63..6e3e1b121 100644 --- a/src/client.ts +++ b/src/client.ts @@ -73,7 +73,8 @@ export class SubscriptionClient { private closedByUser: boolean; private wsImpl: any; private wasKeepAliveReceived: boolean; - private checkConnectionTimeoutId: any; + private tryReconnectTimeoutId: any; + private checkConnectionIntervalId: any; private middlewares: Middleware[]; constructor(url: string, options?: ClientOptions, webSocketImpl?: any) { @@ -380,7 +381,7 @@ export class SubscriptionClient { // send message, or queue it if connection is not open private sendMessageRaw(message: Object) { switch (this.status) { - case this.client.OPEN: + case this.wsImpl.OPEN: let serializedMessage: string = JSON.stringify(message); let parsedMessage: any; try { @@ -391,7 +392,7 @@ export class SubscriptionClient { this.client.send(serializedMessage); break; - case this.client.CONNECTING: + case this.wsImpl.CONNECTING: this.unsentMessagesQueue.push(message); break; @@ -408,7 +409,7 @@ export class SubscriptionClient { } private tryReconnect() { - if (!this.reconnect || this.backoff.attempts > this.reconnectionAttempts) { + if (!this.reconnect || this.backoff.attempts >= this.reconnectionAttempts) { return; } @@ -421,8 +422,13 @@ export class SubscriptionClient { this.reconnecting = true; } + if (this.tryReconnectTimeoutId) { + clearTimeout(this.tryReconnectTimeoutId); + this.tryReconnectTimeoutId = null; + } + const delay = this.backoff.duration(); - setTimeout(() => { + this.tryReconnectTimeoutId = setTimeout(() => { this.connect(); }, delay); } @@ -441,10 +447,22 @@ export class SubscriptionClient { private connect() { this.client = new this.wsImpl(this.url, GRAPHQL_WS); + // Max timeout trying to connect + setTimeout(() => { + if (this.status !== this.wsImpl.OPEN) { + this.close(false, true); + } + }, this.wsTimeout); + this.client.onopen = () => { this.closedByUser = false; this.eventEmitter.emit(this.reconnecting ? 'reconnecting' : 'connecting'); + if (this.tryReconnectTimeoutId) { + clearTimeout(this.tryReconnectTimeoutId); + this.tryReconnectTimeoutId = null; + } + const payload: ConnectionParams = typeof this.connectionParams === 'function' ? this.connectionParams() : this.connectionParams; // Send CONNECTION_INIT message, no need to wait for connection to success (reduce roundtrips) @@ -531,11 +549,11 @@ export class SubscriptionClient { this.checkConnection(); } - if (this.checkConnectionTimeoutId) { - clearTimeout(this.checkConnectionTimeoutId); + if (this.checkConnectionIntervalId) { + clearInterval(this.checkConnectionIntervalId); this.checkConnection(); } - this.checkConnectionTimeoutId = setTimeout(this.checkConnection.bind(this), this.wsTimeout); + this.checkConnectionIntervalId = setInterval(this.checkConnection.bind(this), this.wsTimeout); break; default: diff --git a/src/test/tests.ts b/src/test/tests.ts index b92088370..ef8b07f52 100644 --- a/src/test/tests.ts +++ b/src/test/tests.ts @@ -959,33 +959,28 @@ describe('Client', function () { }); it('should stop trying to reconnect to the server', function (done) { - let connections = 0; wsServer.on('connection', (connection: WebSocket) => { - connections += 1; - if (connections === 1) { - wsServer.close(); - } else { - assert(false); - } + connection.close(); }); const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { - timeout: 100, + timeout: 500, reconnect: true, - reconnectionAttempts: 1, + reconnectionAttempts: 2, }); const connectSpy = sinon.spy(subscriptionsClient, 'connect'); setTimeout(() => { expect(connectSpy.callCount).to.be.equal(2); done(); - }, 500); + }, 1500); }); - it('should stop trying to reconnect if not receives the ack from the server', function (done) { + it('should stop trying to reconnect to the server if it not receives the ack', function (done) { const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { + timeout: 500, reconnect: true, - reconnectionAttempts: 1, + reconnectionAttempts: 2, }); const connectSpy = sinon.spy(subscriptionsClient, 'connect'); wsServer.on('connection', (connection: any) => { @@ -1001,20 +996,23 @@ describe('Client', function () { setTimeout(() => { expect(connectSpy.callCount).to.be.equal(2); done(); - }, 1000); + }, 1500); }); it('should keep trying to reconnect if receives the ack from the server', function (done) { const subscriptionsClient = new SubscriptionClient(`ws://localhost:${RAW_TEST_PORT}/`, { + timeout: 500, reconnect: true, - reconnectionAttempts: 1, + reconnectionAttempts: 2, }); const connectSpy = sinon.spy(subscriptionsClient, 'connect'); + let connections = 0; wsServer.on('connection', (connection: any) => { connection.on('message', (message: any) => { const parsedMessage = JSON.parse(message); // mock server if (parsedMessage.type === MessageTypes.GQL_CONNECTION_INIT) { + ++connections; connection.send(JSON.stringify({ type: MessageTypes.GQL_CONNECTION_ACK, payload: {} })); connection.close(); } @@ -1022,9 +1020,11 @@ describe('Client', function () { }); setTimeout(() => { + expect(connections).to.be.greaterThan(3); expect(connectSpy.callCount).to.be.greaterThan(2); + wsServer.close(); done(); - }, 1000); + }, 1900); }); it('should take care of received keep alive', (done) => { From 8552533131d67e6510d676a9fcba2304f3866d34 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 06:49:39 +0100 Subject: [PATCH 08/14] fix(NA): dont close socket when it isnt open. --- src/client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client.ts b/src/client.ts index 6e3e1b121..2734942f1 100644 --- a/src/client.ts +++ b/src/client.ts @@ -131,7 +131,7 @@ export class SubscriptionClient { this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); } - if (closedByUser) { + if (closedByUser && this.client.status < this.wsImpl.OPEN) { this.client.close(); } this.client = null; @@ -441,7 +441,7 @@ export class SubscriptionClient { } private checkConnection() { - this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false); + this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false, true); } private connect() { From d5793874366c6a9169a97ae002b5c5f1ff125305 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 08:00:34 +0100 Subject: [PATCH 09/14] fix(NA): close socket. --- src/client.ts | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/client.ts b/src/client.ts index 2734942f1..134ea68eb 100644 --- a/src/client.ts +++ b/src/client.ts @@ -75,6 +75,7 @@ export class SubscriptionClient { private wasKeepAliveReceived: boolean; private tryReconnectTimeoutId: any; private checkConnectionIntervalId: any; + private maxConnectTimeoutId: any; private middlewares: Middleware[]; constructor(url: string, options?: ClientOptions, webSocketImpl?: any) { @@ -128,10 +129,24 @@ export class SubscriptionClient { this.closedByUser = closedByUser; if (isForced) { + if (this.checkConnectionIntervalId) { + clearInterval(this.checkConnectionIntervalId); + } + + if (this.maxConnectTimeoutId) { + clearTimeout(this.maxConnectTimeoutId); + this.maxConnectTimeoutId = null; + } + + if (this.tryReconnectTimeoutId) { + clearTimeout(this.tryReconnectTimeoutId); + this.tryReconnectTimeoutId = null; + } + this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); } - if (closedByUser && this.client.status < this.wsImpl.OPEN) { + if (closedByUser) { this.client.close(); } this.client = null; @@ -444,15 +459,24 @@ export class SubscriptionClient { this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false, true); } - private connect() { - this.client = new this.wsImpl(this.url, GRAPHQL_WS); + private checkMaxConnectTimeout() { + if (this.maxConnectTimeoutId) { + clearTimeout(this.maxConnectTimeoutId); + this.maxConnectTimeoutId = null; + } // Max timeout trying to connect - setTimeout(() => { + this.maxConnectTimeoutId = setTimeout(() => { if (this.status !== this.wsImpl.OPEN) { this.close(false, true); } }, this.wsTimeout); + } + + private connect() { + this.client = new this.wsImpl(this.url, GRAPHQL_WS); + + this.checkMaxConnectTimeout(); this.client.onopen = () => { this.closedByUser = false; From 5f598be0fe8e67303080c42ace14b87d66cf0bca Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Tue, 4 Jul 2017 17:12:31 +0100 Subject: [PATCH 10/14] fix(NA): changes according pr. --- src/client.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/client.ts b/src/client.ts index 134ea68eb..5056f49b3 100644 --- a/src/client.ts +++ b/src/client.ts @@ -131,6 +131,7 @@ export class SubscriptionClient { if (isForced) { if (this.checkConnectionIntervalId) { clearInterval(this.checkConnectionIntervalId); + this.checkConnectionIntervalId = null; } if (this.maxConnectTimeoutId) { @@ -146,7 +147,7 @@ export class SubscriptionClient { this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); } - if (closedByUser) { + if (this.client.status === this.wsImpl.OPEN) { this.client.close(); } this.client = null; @@ -456,7 +457,12 @@ export class SubscriptionClient { } private checkConnection() { - this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false, true); + if (this.wasKeepAliveReceived) { + this.wasKeepAliveReceived = false; + return; + } + + this.close(false, true); } private checkMaxConnectTimeout() { From edba02d18a0ba5bc5036c680215a3a6dba55d314 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Wed, 5 Jul 2017 11:45:53 +0100 Subject: [PATCH 11/14] refact(NA): change clear timeouts and intervals code. --- src/client.ts | 60 ++++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/src/client.ts b/src/client.ts index 5056f49b3..a2a5a4897 100644 --- a/src/client.ts +++ b/src/client.ts @@ -129,27 +129,13 @@ export class SubscriptionClient { this.closedByUser = closedByUser; if (isForced) { - if (this.checkConnectionIntervalId) { - clearInterval(this.checkConnectionIntervalId); - this.checkConnectionIntervalId = null; - } - - if (this.maxConnectTimeoutId) { - clearTimeout(this.maxConnectTimeoutId); - this.maxConnectTimeoutId = null; - } - - if (this.tryReconnectTimeoutId) { - clearTimeout(this.tryReconnectTimeoutId); - this.tryReconnectTimeoutId = null; - } - + this.clearCheckConnectionInterval(); + this.clearMaxConnectTimeout(); + this.clearTryReconnectTimeout(); this.sendMessage(undefined, MessageTypes.GQL_CONNECTION_TERMINATE, null); } - if (this.client.status === this.wsImpl.OPEN) { - this.client.close(); - } + this.client.close(); this.client = null; this.eventEmitter.emit('disconnected'); @@ -305,6 +291,27 @@ export class SubscriptionClient { return this; } + private clearCheckConnectionInterval() { + if (this.checkConnectionIntervalId) { + clearInterval(this.checkConnectionIntervalId); + this.checkConnectionIntervalId = null; + } + } + + private clearMaxConnectTimeout() { + if (this.maxConnectTimeoutId) { + clearTimeout(this.maxConnectTimeoutId); + this.maxConnectTimeoutId = null; + } + } + + private clearTryReconnectTimeout() { + if (this.tryReconnectTimeoutId) { + clearTimeout(this.tryReconnectTimeoutId); + this.tryReconnectTimeoutId = null; + } + } + private logWarningOnNonProductionEnv(warning: string) { if (process && process.env && process.env.NODE_ENV !== 'production') { console.warn(warning); @@ -438,10 +445,7 @@ export class SubscriptionClient { this.reconnecting = true; } - if (this.tryReconnectTimeoutId) { - clearTimeout(this.tryReconnectTimeoutId); - this.tryReconnectTimeoutId = null; - } + this.clearTryReconnectTimeout(); const delay = this.backoff.duration(); this.tryReconnectTimeoutId = setTimeout(() => { @@ -466,10 +470,7 @@ export class SubscriptionClient { } private checkMaxConnectTimeout() { - if (this.maxConnectTimeoutId) { - clearTimeout(this.maxConnectTimeoutId); - this.maxConnectTimeoutId = null; - } + this.clearMaxConnectTimeout(); // Max timeout trying to connect this.maxConnectTimeoutId = setTimeout(() => { @@ -487,11 +488,7 @@ export class SubscriptionClient { this.client.onopen = () => { this.closedByUser = false; this.eventEmitter.emit(this.reconnecting ? 'reconnecting' : 'connecting'); - - if (this.tryReconnectTimeoutId) { - clearTimeout(this.tryReconnectTimeoutId); - this.tryReconnectTimeoutId = null; - } + this.clearMaxConnectTimeout(); const payload: ConnectionParams = typeof this.connectionParams === 'function' ? this.connectionParams() : this.connectionParams; @@ -509,7 +506,6 @@ export class SubscriptionClient { this.client.onerror = () => { // Capture and ignore errors to prevent unhandled exceptions, wait for // onclose to fire before attempting a reconnect. - this.close(false, true); }; this.client.onmessage = ({ data }: {data: any}) => { From b3a65a8de17dc94e4ad7b01cfd27fdbdcd1a76c6 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Wed, 5 Jul 2017 15:26:45 +0100 Subject: [PATCH 12/14] fix(NA): dont close connection with check connection when reconnect. --- src/client.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/client.ts b/src/client.ts index a2a5a4897..8dad2763e 100644 --- a/src/client.ts +++ b/src/client.ts @@ -466,7 +466,9 @@ export class SubscriptionClient { return; } - this.close(false, true); + if (!this.reconnecting) { + this.close(false, true); + } } private checkMaxConnectTimeout() { @@ -486,9 +488,9 @@ export class SubscriptionClient { this.checkMaxConnectTimeout(); this.client.onopen = () => { + this.clearMaxConnectTimeout(); this.closedByUser = false; this.eventEmitter.emit(this.reconnecting ? 'reconnecting' : 'connecting'); - this.clearMaxConnectTimeout(); const payload: ConnectionParams = typeof this.connectionParams === 'function' ? this.connectionParams() : this.connectionParams; From 2963859640c900229f581607263ffb4b56edefb5 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Wed, 5 Jul 2017 15:32:37 +0100 Subject: [PATCH 13/14] docs(NA): changelog updated. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0a18baaf..196374db0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog ### vNEXT +- Fix for non forced closes (now it wont send connection_terminate) [PR #197](https://github.com/apollographql/subscriptions-transport-ws/pull/197) +- A lot of connection's flow improvements (on connect, on disconnect and on reconnect) [PR #197](https://github.com/apollographql/subscriptions-transport-ws/pull/197) ### 0.7.3 - Fix for first subscription is never unsubscribed [PR #179](https://github.com/apollographql/subscriptions-transport-ws/pull/179) From 97c0fc086bcf27c558fee11c81e99a9a7f142c19 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 7 Jul 2017 03:47:58 +0100 Subject: [PATCH 14/14] feat(NA): added max connect time exponentional generator. --- README.md | 4 ++-- src/client.ts | 16 +++++++++++++++- src/defaults.ts | 2 +- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 83a19b610..de672ffed 100644 --- a/README.md +++ b/README.md @@ -216,9 +216,9 @@ ReactDOM.render( ### `Constructor(url, options, connectionCallback)` - `url: string` : url that the client will connect to, starts with `ws://` or `wss://` - `options?: Object` : optional, object to modify default client behavior - * `timeout?: number` : how long the client should wait in ms for a keep-alive message from the server (default 30000 ms), this parameter is ignored if the server does not send keep-alive messages. + * `timeout?: number` : how long the client should wait in ms for a keep-alive message from the server (default 10000 ms), this parameter is ignored if the server does not send keep-alive messages. This will also be used to calculate the max connection time per connect/reconnect * `lazy?: boolean` : use to set lazy mode - connects only when first subscription created, and delay the socket initialization - * `connectionParams?: Object | Function` : object that will be available as first argument of `onConnect` (in server side), if passed a function - it will call it and send the return value. + * `connectionParams?: Object | Function` : object that will be available as first argument of `onConnect` (in server side), if passed a function - it will call it and send the return value * `reconnect?: boolean` : automatic reconnect in case of connection error * `reconnectionAttempts?: number` : how much reconnect attempts * `connectionCallback?: (error) => {}` : optional, callback that called after the first init message, with the error (if there is one) diff --git a/src/client.ts b/src/client.ts index 8dad2763e..8fc73c8f9 100644 --- a/src/client.ts +++ b/src/client.ts @@ -77,6 +77,7 @@ export class SubscriptionClient { private checkConnectionIntervalId: any; private maxConnectTimeoutId: any; private middlewares: Middleware[]; + private maxConnectTimeGenerator: any; constructor(url: string, options?: ClientOptions, webSocketImpl?: any) { const { @@ -110,6 +111,7 @@ export class SubscriptionClient { this.eventEmitter = new EventEmitter(); this.middlewares = []; this.client = null; + this.maxConnectTimeGenerator = this.createMaxConnectTimeGenerator(); if (!this.lazy) { this.connect(); @@ -291,6 +293,17 @@ export class SubscriptionClient { return this; } + private createMaxConnectTimeGenerator() { + const minValue = 1000; + const maxValue = this.wsTimeout; + + return new Backoff({ + min: minValue, + max: maxValue, + factor: 1.2, + }); + } + private clearCheckConnectionInterval() { if (this.checkConnectionIntervalId) { clearInterval(this.checkConnectionIntervalId); @@ -479,7 +492,7 @@ export class SubscriptionClient { if (this.status !== this.wsImpl.OPEN) { this.close(false, true); } - }, this.wsTimeout); + }, this.maxConnectTimeGenerator.duration()); } private connect() { @@ -548,6 +561,7 @@ export class SubscriptionClient { this.eventEmitter.emit(this.reconnecting ? 'reconnected' : 'connected'); this.reconnecting = false; this.backoff.reset(); + this.maxConnectTimeGenerator.reset(); if (this.connectionCallback) { this.connectionCallback(); diff --git a/src/defaults.ts b/src/defaults.ts index b1ec67817..abf60e860 100644 --- a/src/defaults.ts +++ b/src/defaults.ts @@ -1,4 +1,4 @@ -const WS_TIMEOUT = 30000; +const WS_TIMEOUT = 10000; export { WS_TIMEOUT,