Feature: inactivity timeout and tests #390
Conversation
|
Fixes #351 |
|
Yeah! This is really cool! Looks good to me! |
src/client.ts
Outdated
| @@ -167,6 +174,9 @@ export class SubscriptionClient { | |||
|
|
|||
| let opId: string; | |||
|
|
|||
| this.activeSubscriptions += 1; | |||
There was a problem hiding this comment.
This should be next to where this.operations[opId] = is done to ensure the reference counting it done correctly. Or possibly the check should just be against Object.keys(this.operations).length to avoid needing to do reference counting.
There was a problem hiding this comment.
Done, we are not counting activeSubscriptions anymore and relying on Object.keys(this.operations).length to test inactivity.
| } | ||
| } | ||
|
|
||
| private setInactivityTimeout() { |
There was a problem hiding this comment.
What should the behavior of this function be as it related to lazy = false since unsubscribe is the only place it gets called.
There was a problem hiding this comment.
Should this function also test for connection to exist? I think this function is idempotent as is and can be called in any state. Not sure on the impact of lazy flag on this? Can you please elaborate.
src/client.ts
Outdated
| if (this.inactivityTimeout > 0 && this.activeSubscriptions === 0) { | ||
| this.inactivityTimeoutId = setTimeout(() => { | ||
| if (this.activeSubscriptions === 0) { | ||
| this.close(); |
There was a problem hiding this comment.
Should add a test to verify that if the connection is closed due to inactivity, a future subscribe will correctly reopen the connection.
There was a problem hiding this comment.
Thanks, added the relevant test case.
| @@ -352,6 +362,23 @@ export class SubscriptionClient { | |||
| } | |||
| } | |||
|
|
|||
| private clearInactivityTimeout() { | |||
There was a problem hiding this comment.
Should there be a call to this if the connection is closed for any other reason, otherwise I could see the reconnect logic fighting with the inactivity timeout and possibly tripping the flags for closedByUser.
There was a problem hiding this comment.
I agree, if a connection is closed (say, manually) but inactivity timeout had started. It might close the connection after reconnection.
To get around this, I called clearInactivityTimeout inside the close function.
Based on the feedback on the PR, this commit does the following:- 1. Rely on `Object.keys(this.operations).length` to detect inactivity in place of manually counting active subscriptions. 2. Call `clearInactivityTimeout` on connection close to handle the case when connection is closed for another reason than inactivity timeout, it should not mess with reconnect logic and other related operations. 3. Add test case to validate that reconnection on subscribe works after a connection was closed due to inactivity.
|
@NeoPhi : Thanks for the feedback
Please let me know if any further changes are needed. |
|
@NeoPhi any news on this? Sorry for bothering, but we really need this to be merged as all people using server-side subscriptions with Yoga have connection leaks right now. |
|
@timsuchanek I would contend that if a well behaved client is causing memory leaks in Yoga, that sounds like a server side issue. Long lived idle WebSockets are fine. |
Pass through unknown errors
* Add inactivityTimeout option to subscription client 1. Passed through websocket link 2. Requires bump in version after this is merged apollographql/subscriptions-transport-ws#390 * bump version of subscriptions-transport-ws to 0.9.8
TODO: