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.

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

Fix cleanup observer blocking unsubscribe (#6985) #7165

Conversation

javier-garcia-meteologica
Copy link
Contributor

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

Fixes #6985

Cleanup observers were blocking removeObserver from executing this.sub.unsubscribe(). Therefore some queries were left hanging until completion. This patch puts the observers that should not block unsubscription into a separate set, shadowObservers.

@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/

@benjamn benjamn changed the base branch from main to release-3.3 October 14, 2020 21:38
@dylanwulf
Copy link
Contributor

May also fix #4955

benjamn added a commit to javier-garcia-meteologica/apollo-client that referenced this pull request Oct 14, 2020
When there are multiple observers subscribed to a Concast, messages should
be delivered in the order the observers were added.

The shadowObservers approach notified any cleanup observers after any
normal observers, potentially reordereding message delivery. I'm not sure
that reordering was necessarily "wrong," but it would be a very tricky
problem to track down for anyone who might be affected by it.

However, if any other non-cleanup observers have been added, and the last
observer removed is a cleanup observer, then we should unsubscribe from
this.sub, to preserve the fix for apollographql#6985.
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 Thanks for fixing this!

I'm going to go ahead and merge this PR into release-3.3 and publish another beta (so you can try these changes out), but please do let me know if you disagree with any of the changes I made.

@benjamn benjamn merged commit 2b8ee34 into apollographql:release-3.3 Oct 14, 2020
@javier-garcia-meteologica
Copy link
Contributor Author

javier-garcia-meteologica commented Oct 15, 2020

Hi @benjamn

I've tested the changes and unfortunately queries are not being aborted. I've open a PR to fix it in #7170.

The problem is that there are still 2 observers when removeObserver is called: the query observer and the cleanup observer.

  public removeObserver(observer: Observer<T>, quietly?: boolean) {
    if (this.observers.delete(observer) &&
        this.observers.size < 1) {
      if (quietly) return;
      if (this.sub) {
        this.sub.unsubscribe();

This is easy to fix, just decrement --this.addCount and check this.addCount < 1.

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.

Unsubscribing from a query does not abort network requests
4 participants