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

Polling stops on error #1099

Closed
dallonf opened this issue Dec 27, 2016 · 9 comments
Closed

Polling stops on error #1099

dallonf opened this issue Dec 27, 2016 · 9 comments
Labels
📚 good-first-issue Issues that are suitable for first-time contributors. 🐞 bug

Comments

@dallonf
Copy link

dallonf commented Dec 27, 2016

I'm currently trying to set up a polling process to check the status of a long operation, here's some example code:

const waitForCompletionQuery = gql`
query WaitForCloneCompletion {
  operationStatus {
    isCompleted
  }
}
`;

async function waitForCompletion(apolloClient) {
  const query = apolloClient.watchQuery({
    query: waitForCompletionQuery,
    forceFetch: true,
    pollInterval: 5000
  });
  let subscription;
  try {
    await new Promise((resolve, reject) => {
      const observer = {
        error: (err) => {
          if (isOKError(err)) {
            // Alert the user to the error and keep trying
            showErrorToast(err);
          } else {
            reject(err);
          }
        },
        next: (result) => {
          if (result.loading) return;
          const status = result.data.operationStatus;
          if (status.isCompleted) {
            resolve();
          }
        }
      };
      subscription = query.subscribe(observer);
    });
  } finally {
    if (subscription) {
      subscription.unsubscribe();
    }
  }
}

Some errors would be considered "OK" (like network errors, which probably makes this at least somewhat related to #270) - while I need to make them known to the user, I should keep polling. Others indicate an operation failure and I should stop polling because it's not going to change.

I would expect the above code to behave in that way, returning a promise that resolves on the first isCompleted value or rejects on the first non-OK error, but what actually appears to happen is that the polling just stops after the first error, and won't start back up even if I explicitly call query.startPolling(5000) or subscription = query.subscribe(observer) after showErrorToast().

Is this user error or something odd in Apollo?

@helfer
Copy link
Contributor

helfer commented Dec 30, 2016

@dallonf I think it's likely that it's a bug in Apollo Client. My best guess is that the client still thinks it's polling, so it doesn't start a new polling query when the first one fails.

If not like #270 (i.e. just keep retrying/polling), how would you expect to handle errors? Would it be okay in a first step to just make sure you can start the polling again after an error has happened?

@dallonf
Copy link
Author

dallonf commented Dec 30, 2016

My expectation of default behavior is that polling would go on forever until explicitly stopped (which in most higher-level usage, happens when the subscribed component unmounts), so I would also expect the error callback to be called for each new error (although it would also make some sense if it only was called for different errors, like next is only called for new data).

But if the behavior of automatically stopping a poll on error was considered expected and documented, then the ability to re-subscribe would definitely unblock my use case.

@longuniquename
Copy link

Have the same issue. Any progress on this? Thanks.

@calebmer calebmer added 🐞 bug 📚 good-first-issue Issues that are suitable for first-time contributors. labels Jan 20, 2017
@jonathanheilmann
Copy link

The same issue appears, if I subscribe to a watchQuery and http status is something like 4xx. onError of my subscription is called, but actually I can't see any option to unsubscribe. apollo-client is still listening for changes in store (reducer is called), but changes won't be reflected to my subscription. This leads me to some errors if i try to reset apollo store (on logout for example).

Is there any option to unsubscribe on error, or any other solution?
Maybe it's a solution to provide a more dedicated error handling via Observable.catch() or similar?

@daniman
Copy link
Member

daniman commented Mar 7, 2017

@helfer can we bump this? :)

For transient errors that are acceptable while polling, like network errors, it'd be nice if AC continued to poll and handled those errors elegantly. For cases where you'd want to display a network error toast or something, we could expose the state/error on the network status that indicates that the query is not actively polling successfully.

@helfer
Copy link
Contributor

helfer commented May 6, 2017

I want to provide a bit of context to explain why we haven't done anything about this so far.

Right now watchQuery returns an ObservableQuery, which is just an observable with some extra functions. We call error on subscribers when an error happens. Observables are only allowed to call error once, which means we can't keep polling after delivering the first error. So we'd have to choose either between not delivering the error, or stopping the polling.

It's not great, but short of changing how errors are delivered in all of Apollo Client, our hands are kind of tied for this one.

@dallonf @daniman would you find an option to keep polling on error useful? In that case the polling query would continue without throwing an error (if the error happens on the first query, it would still throw an error). Let me know your thoughts!

Edit: on second thought, this may be something that can be handled much more easily in react-apollo. It could simply restart the polling query after a timeout if it failed. That way it could also return data along with errors to the component. If you think that sounds interesting, please open an issue on react-apollo and refer to this issue! 🙂

@helfer helfer closed this as completed May 6, 2017
@dallonf
Copy link
Author

dallonf commented May 6, 2017

I think this should definitely be something done in React Apollo, but it doesn't really help this particular use case since it was using Apollo Client directly. I think the biggest problem is that, at least when I created this ticket, polling would not restart even if you call startPolling() again; was this fixed?

@helfer
Copy link
Contributor

helfer commented May 6, 2017

@dallonf I see, that's unfortunate. I don't think there's anything we can do to restart the polling once the observable has errored for the same reason that I mentioned above. Maybe we should consider using observables that can emit multiple errors though. Right now if you want to continue polling, you have to start a new query.

@slonoed
Copy link

slonoed commented Jan 22, 2018

@helfer could you provide a minimal example of how to start a new query? Thanks in advance!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📚 good-first-issue Issues that are suitable for first-time contributors. 🐞 bug
Projects
None yet
Development

No branches or pull requests

7 participants