[4.0] [Regiression] Unsubscibing from ObservableQuery no longer aborts in flight request

[wassim-k]

Hi,

While testing Apollo Client v4, I noticed that when calling setVariables on a watched observable query, it no longer aborts in flight http requests. The same is true for simply unsubscribing while the request is in progress.
This behaviour has changed since v3 which called AbortController.abort as expected.

We have some heavy data analytics queries that can run for a long period of time, being able to cancel those if the user changes the paramters or navigates away reduces the load on the server.

There is a comment here explaining the bug, so it seems like that might have been done intentionally, do you mind shedding some light on the thinking behind it.

Thanks.

[jerelmiller]

Hey @wassim-k 👋

This was an intentional change done in 4.0.0-alpha.0 and called out in the changelog:

#12384 6aa6fd3 Thanks @jerelmiller! - Unusubscribing from ObservableQuery while a request is in flight will no longer terminate the request by unsubscribing from the link observable.

This affects any API that calls reobserve, such as setVariables. The reason for this was to better align our React hooks and core behavior. The useLazyQuery hook, which executes queries when calling the execute function, previously allowed for requests to complete even after the hook was unmounted (i.e. it fully unsubscribed from the underlying ObservableQuery). This was a change fixed in 3.7.11 and we felt we wanted alignment on this behavior client-wide rather than a one-off place.

That said, it probably makes sense for us to allow a way for this to happen. Let me take this to the team and throw around some ideas. Appreciate the feedback here! I'll add this to the 4.0 milestone so that we don't lose track of this.

[wassim-k]

Hey @jerelmiller, again thanks for taking the time to explain.

Given that GraphQL queries are side-effect free in nature, cancelling them when they're no longer needed (when the user navigates away or sets new variables) is generally a safe operation, especially in cases, as explained above, when the query is resource intensive and can take up to 30s in processing, this can have a noticeable impact on the server load.

Furthermore, Angular's built-in HttpClient returns an observable which similarly, when unsubscribed, cancels the in-flight request, so that to me feels like the default expected behavior from a client.

That being said, looking at the reason this change was introduced, I understand why it was necessary.

Now, forgive my limited React knowledge, but my understanding is that the crux of the problem boils down to the fact that a rejected execute promise can cause the ErrorBoundary to dismount the component before being able to handle the result.

I may have a suggestion, and let me know if this is too big of a breaking change.
But since execute returns a Promise<QueryResult<T>> and QueryResult already has an error field, what if the promise returned from execute always resolves to a QueryResult by design:

return promise.catch(error => ({ data: undefined, error }));

and this way it simplifies the use of the API, since the caller can simply:

const { data, error } = await execute();

if (error) {
  handleError(error);
}

and deal with the error (including abort errors) safely without worrying about wraping it in a try...catch.

If this is still not an acceptable solution, I would appreciate having the option to control this behavior at the client level, as it would be considered quite a big breaking change for us.

Thanks.

[jerelmiller]

Now, forgive my limited React knowledge, but my understanding is that the crux of the problem boils down to the fact that a rejected execute promise can cause the ErrorBoundary to dismount the component before being able to handle the result.

It's actually a bit more complicated than that 😆. If you want some reading material, check out the comments in the issues of the following (I've got quite a few comments in those issues that explain the background behind these changes in much more detail):

The original issue was reported by #10365 because unmounting a component with useLazyQuery left the promise forever pending. That originally led to fix it with #10427 which aborted in flight requests, but that turned out to be a bit spicy as it was causing significantly more noise than expected (#10533, #10520). This led to #10698 which ultimately made it so that requests resolved naturally rather than aborting the request.

Given that GraphQL queries are side-effect free in nature, cancelling them when they're no longer needed (when the user navigates away or sets new variables) is generally a safe operation, especially in cases, as explained above, when the query is resource intensive and can take up to 30s in processing, this can have a noticeable impact on the server load.

Totally agree. I had a chance to catch up with the team today and we decided that it makes sense to have some way to reintroduce this behavior. I'll be noodling some more on this tomorrow and hope to have something up very soon. Whatever we end up with will need to account for the useLazyQuery case though to ensure we can toggle that behavior on/off (this is likely to end up as a user option in some form anyways 🙂)

let me know if this is too big of a breaking change.

Since its a major, this is a perfect time for breaking changes 😆! We are ok introducing breaking changes between alphas since that is what alphas are for. Keep the feedback coming!

since execute returns a Promise<QueryResult> and QueryResult already has an error field, what if the promise returned from execute always resolves to a QueryResult by design:

The only issue I have here is that we'd like to ensure errorPolicy plays a role here. One of the things we have done with 4.0 is ensure that errorPolicy behaves much more predictably between error types. 3.x always rejected the promise if the error was a network error, regardless of errorPolicy. In 4.0, errorPolicy works the same across the board, so to keep that consistency, we'll want any abort errors to reject the promise returned from the promise if errorPolicy is none. If it is all or ignore, then the promise will resolve with the error property set accordingly.

I would appreciate having the option to control this behavior at the client level

We will definitely add something back in! Again, I don't know exactly how this will look yet, but we'll get something in place.

Once again, appreciate the feedback!