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

Conversation

tretne
Copy link
Contributor

@tretne tretne commented Aug 5, 2019

Resolves #581

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@apollo-cla
Copy link

@tretne: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks @tretne!

@hwillson hwillson merged commit 2e4bb91 into apollographql:master Aug 10, 2020
@@ -167,6 +167,10 @@ export class SubscriptionClient {
}

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

@taras-slapdash taras-slapdash Sep 4, 2020

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?

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.

@ghost ghost mentioned this pull request Sep 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebSocket event listeners not cleaned up after WebSocket is closed
6 participants