Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Clear WebSocket event listeners on close. #615

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Clear WebSocket event listeners on close. #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ed7bca
Clear WebSocket event listeners on close.
tretne Aug 5, 2019
0348bb6
Changelog update
hwillson Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- Do not send GQL_STOP when unsubscribing after GQL_COMPLETE is received. <br/>
[@onhate](https://github.com/onhate) in [#775](https://github.com/apollographql/subscriptions-transport-ws/pull/775)
- Clear WebSocket event listeners on close. <br/>
[@tretne](https://github.com/tretne) in [#615](https://github.com/apollographql/subscriptions-transport-ws/pull/615)

### New Features

Expand Down
4 changes: 4 additions & 0 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ export class SubscriptionClient {
}

this.client.close();
this.client.onopen = null;
Copy link

@taras-slapdash taras-slapdash Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

After this change we started getting the following error in our app:

[TypeError: Cannot set property 'onopen' of null]

e.close (webpack://[name]/node_modules/subscriptions-transport-ws/dist/client.js:126:33)
close (webpack://[name]/node_modules/subscriptions-transport-ws/dist/client.js:477:22)

I believe it is due to some race conditioning where two simultaneous calls are made to close().

Unfortunately, I don't have a repro for this bug so it's tricky to tell exactly what went wrong.

The call stack starts in the this.client.onclose handler (see https://github.com/apollographql/subscriptions-transport-ws/blob/v0.9.18/src/client.ts#L580-L584).

Any ideas?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tretne I think this Cannot set property 'onopen' of null error happens when close() is called synchronously from within close() in a nested way (my guess - during this.unsubscribeAll() call - the subscription handlers may have some cleanups within themselves which close the the client).

The solution would be to either:

  1. have one more check before using this.client on L169 and below, or
  2. assign null to this.client early (if it's applicable).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think @dko-slapdash is right -- I'm able to reproduce this pretty easily by loading my app with the network down.
e.g.

  1. Host backend locally
  2. Point ngrok at the backend
  3. Point the client at ngrok

Refresh the app.

A solution is:

const client = this.client
this.client = null
// rest of cleanup ...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#829

this.client.onclose = null;
this.client.onerror = null;
this.client.onmessage = null;
this.client = null;
this.eventEmitter.emit('disconnected');

Expand Down