Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix cleanup observer blocking unsubscribe (2) (#6985) #7170

Merged
merged 3 commits into from
Oct 16, 2020
Merged

Fix cleanup observer blocking unsubscribe (2) (#6985) #7170

merged 3 commits into from
Oct 16, 2020

Conversation

javier-garcia-meteologica
Copy link
Contributor

This patch decrements the addCount tally so this.sub can be unsubscribed and queries are aborted.

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@apollo-cla
Copy link

@javier-garcia-meteologica: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@javier-garcia-meteologica javier-garcia-meteologica mentioned this pull request Oct 15, 2020
Merged
2 tasks
@javier-garcia-meteologica javier-garcia-meteologica marked this pull request as draft October 15, 2020 10:05
@javier-garcia-meteologica javier-garcia-meteologica marked this pull request as ready for review October 15, 2020 13:00
@dotansimha
Copy link
Contributor

dotansimha commented Oct 15, 2020 •
edited
Loading

This is great! Really waiting for this to get merged. We noticed another issue we are investigating now, seems like it's related to GraphQL subscriptions. We see some weak references that retain in memory. It might be addressed by this as well.

@benjamn
Simplify concast.addCount logic and unsubscribe asynchronously.
4ce6195 
The test failure in apollographql#7170 was a case of unsubscribing the last observer
and then immediately resubscribing another observer, which should keep the
subscription alive, for backwards compatibility. Delaying the unsubscribe
to a later microtask tick enables this use case, while still unsubscribing
very soon after the last non-cleanup Concast observer unsubscribes.

In case there are still any cleanup observers in this.observers, and no
error or completion has been broadcast yet, we call this.handlers.error to
make sure those cleanup observers receive an error that terminates them.
@benjamn
Copy link
Member

benjamn commented Oct 15, 2020

@javier-garcia-meteologica Thanks for adding a test!

I was able to fix the test failure by adding a Promise.resolve().then delay to the sub.unsubscribe() call, which seems important for backwards compatibility (so unsubscribing the last observer and then immediately subscribing another observer does not terminate the underlying subscription).

I also noticed that we were not always terminating cleanup observers if there are any left in this.observers when the last non-cleanup observer is removed by removeObserver, so now that's handled better.

@benjamn benjamn added this to the Post 3.0 milestone Oct 15, 2020
benjamn
benjamn requested changes Oct 15, 2020
View reviewed changes
Copy link
Member

@benjamn benjamn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@javier-garcia-meteologica I think this is close, but I'd like to see if we can avoid the changes to MockLink (see #7170 (comment)). What do you think?

benjamn
benjamn approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

@javier-garcia-meteologica Perfect, thanks!

@benjamn benjamn merged commit fd1d03a into apollographql:release-3.3 Oct 16, 2020
benjamn added a commit that referenced this pull request Oct 16, 2020
@benjamn
Mention PRs #7161 and #7170 in CHANGELOG.md.
f592167
@dotansimha dotansimha mentioned this pull request Oct 22, 2020
Merged
This was referenced Mar 16, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benjamn benjamn benjamn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Post 3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@javier-garcia-meteologica @apollo-cla @dotansimha @benjamn