apollographql / apollo-feature-requests

🧑‍🚀 Apollo Client Feature Requests | (no 🐛 please).
Other
130 stars 7 forks source link

Feature idea: Abort pending requests #362

Open klis87 opened 6 years ago

klis87 commented 6 years ago

Migrated from: apollographql/apollo-client#1541

I think that request aborts and other things from migated topics like options borrowed from Redux-Saga, for example takeLatest (cancelling previous request if there is new pending) could be easier implemented once race lands into Apollo Links, like mentioned in https://www.apollographql.com/docs/link/composition.html

matthargett commented 5 years ago

This is really critical on systems where the pool of sockets is limited, and where CPU is limited. If the user has navigated away before we have the data, ideally the socket (or stream in the case of H/2, or subscription in the case of pubsub like redis) gets closed and cleaned up immediately so a queued request that was waiting for a free socket can get its turn. Additionally, JSON/responses for a cancelled request shouldn’t take up CPU time being parsed/deserialized. This was a key optimization with redux + RxJS allowed for in one of the teams I worked on.

SirwanAfifi commented 5 years ago

Any progress on this? In our project we have a react component to upload multiple images, now we want to add image cancelation ability, I had a look at the source code, it seems that we can pass a fetchOptions using mutation function:

const { mutation: uploadImg } = uploadImageObject;
uploadImg({
   variables: {
     file
   },
   context: {
     fetchOptions: {
      signal,
     }
   }
});

But when I trigger the abort controller, nothing happens, it actually doesn't stop pending request. Any idea?

rico345100 commented 5 years ago

How to use this? How to use stopQuery and what is queryId? Could you make some explanation or simple example?

amcdnl commented 5 years ago

+1 for abort - for things like typeahead this reduces race cases.

firasdib commented 5 years ago

Does anyone have any more information about this? I'm seeing the same issue as @SirwanAfifi.

januszhou commented 5 years ago

this works for me at apollo-client@2.5.1

const abortController = new AbortController();
client.query({ 
  query,
  variables,
  context: { 
    fetchOptions: { 
      signal: abortController.signal 
    }
});
// later on
abortController.abort();

EDIT: This solution works for the first time, but it doesn't cancel for the second request, looks like https://github.com/apollographql/apollo-client/issues/4150 had a solution(not tested yet)

gabor commented 5 years ago

check this discussion https://github.com/apollographql/apollo-client/issues/4150#issuecomment-487588145 , it has a solution for canceling requests.

mrdulin commented 5 years ago

My case is when I use schema stitching, here is my code:

const http = ApolloLink.from([
    new RetryLink({
      attempts: {
        max: 5,
        retryIf: (error, operation: Operation) => {
          logger.error(`retry to connect error: ${error.message}`);
          return !!error;
        },
      },
      delay: {
        initial: 500,
        jitter: true,
        max: Infinity,
      },
    }),
    new HttpLink({ uri, fetch }),
  ]);

  const schemaOriginal = makeRemoteExecutableSchema({
    schema: await introspectSchema(http),
    link: http,
  });

When the remote service is down or not found, I will retry 5 times and after that, I want to stop/abort retrying.

mnpenner commented 5 years ago

@januszhou Where are you seeing fetchOptions as an option? https://www.apollographql.com/docs/react/api/apollo-client/#ApolloClient.query

Edit: It does seem to work.

januszhou commented 5 years ago

@januszhou Where are you seeing fetchOptions as an option? https://www.apollographql.com/docs/react/api/apollo-client/#ApolloClient.query

Edit: It does seem to work.

My solution turned out doesn't work the second request, looks like https://github.com/apollographql/apollo-client/issues/4150 has a better solution(I haven't tested it yet)

mnpenner commented 5 years ago

@januszhou Thanks! I was noticing some weirdness when I did this. I had a .finally() which was never been called after I aborted a query, probably because it was never being re-executed.

NikitaDev14 commented 5 years ago

I had a problem with race condition using RxJs+Apollo and I created a simple solution, hope it will be useful. graphql-express

bgultekin commented 4 years ago

It looks like also client.stop() helps for aborting pending requests.

const client = new ApolloClient({
    link: new HttpLink({
        uri: 'http://graphql.url'
    }),
    queryDeduplication: false
});

// some queries run

client.stop();

If you don't want to abort everything sent through the client, you can use client.queryManager.stopQuery(queryId) (this is what client.stop() uses at the end and it is reachable, but haven't tested).

devdudeio commented 4 years ago

How can I get the current queryId ?

bgultekin commented 4 years ago

@rlech Haven't tested but it seems like you can check client.queryManager.queries map after sending a query. You may find a better answer in the QueryManager class (https://github.com/apollographql/apollo-client/blob/65eef3730e8b3d2b66ca0fe6d88d0b579a4d31ea/packages/apollo-client/src/core/QueryManager.ts).

hueter commented 4 years ago

This would be incredibly useful and powerful as part of the hooks API, for instance something like:

const { data, loading, fetchMore, refetch, cancel } = useQuery(ANY_QUERY);

useEffect(() => {
  return () => {
    // aka componentWillUnmount
    cancel();
  };
}, [cancel]);

Without this, as others have mentioned, typeahead components (e.g. ReactSelect) and other async components are susceptible to the Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. issue

devdudeio commented 4 years ago

@hueter yeah I was looking for something like this. Would be awesome when we see this in the near future.

TSMMark commented 4 years ago

What would it take to implement? Is anyone already taking a try at a PR?

alflennik commented 4 years ago

I need this.

We have a feature where admins can log in as different users, and when they do we invalidate and refetch the GraphQL graph by calling await client.resetStore(). This does clear and refetch all the completed queries, but the problem is that there are pending requests which will come back with data for the wrong user!

I am so sure that I can call some of the APIs on the client to invalidate the requests, but everything I've tried hasn't worked. Even if there is a low-level API I can use, this seems like a pretty natural issue to run into, so a high level API would be save the next person a lot of effort.

qortex commented 4 years ago

Faced the same issue with fetchMore (implementing a typeahead for "search" field: without debounce on the keyboard inputs, some requests have race conditions which brought up the issue).

I did not find a way to gracefully plug a switchMap because the updateQuery function is called directly and not through an Observable chain - which is unfortunate.

I did the following ugly hack, which works in my case because I can keep track of the sent fetchMore queries. I add an artificial new random variable to all fetchMore queries I do and remember what was the last one. I accept the results in updateQuery only if the result comes from the last one.

this.lastFilterQuerySent = Math.floor(Math.random() * 100000000);

targetQuery.fetchMore({
      variables: {
        // ... other variables
        _internalSendingId: this.lastFilterQuerySent,
      },
      updateQuery: (prev, result) => {
        if (
          !result.fetchMoreResult ||
          result.variables._internalSendingId !== this.lastFilterQuerySent
        ) {
          // discard since it's not the last query
          return prev;
        }
        // ok take into account
        return result.fetchMoreResult;
      },
    });

It does the trick, but with several serious drawbacks:

Really, just a "cancel inflight" would be great, since the checkInFlight attribute properly pinpoints when a request for this query is already flying - but stop stops the query entirely, making it impossible to follow up with another call.

Or a simple way to plug a switchMap call based on when the fetchMore call takes place.

(Unsubscribing from the observable as suggested is not acceptable for my use case, since I don't own the subscribers to the data source in this context).

qortex commented 4 years ago

Actually, the hack above can be improved: no need to store the internalSendingId in the query variables: taking advantage of closures and local variables scopes, it can be simplified to:

this.lastFilterQuerySent = Math.floor(Math.random() * 100000000);
const localQuerySentId = this.lastFilterQuerySent;

targetQuery.fetchMore({
      variables: {
        // ... other variables
      },
      updateQuery: (prev, result) => {
        if (
          !result.fetchMoreResult ||
          localQuerySentId !== this.lastFilterQuerySent
        ) {
          // discard since it's not the last query
          return prev;
        }
        // ok take into account
        return result.fetchMoreResult;
      },
    });

It still smells, but a bit less: caching strategies can apply normally because the variables are not touched (although it seems fetchMore calls are never cached as far as I can see in my testing examples).

dannycochran commented 4 years ago

For the useQuery abort case, there's a fairly complex way of making this work with your Apollo Client if anyone is looking for short term relief. This is what I'm doing and it works well for the use case where I want to cancel previous queries from a text input.

  1. First, set up your apollo client with an observable that will keep track of in flight requests and if a new request comes in from the same component, cancel the previous one:
const requestLink = new ApolloLink(
  (operation, forward) =>
    new Observable(observer => {
      // Set x-CSRF token (not related to abort use case)
      let handle: ZenObservable.Subscription | undefined;
      Promise.resolve(operation)
        .then(oper => request(oper))
        .then(() => {
          handle = forward(operation).subscribe({
            next: observer.next.bind(observer),
            error: observer.error.bind(observer),
            complete: observer.complete.bind(observer),
          });
        })
        .catch(observer.error.bind(observer));

      const context = operation.getContext();
      const requestId = uuid();

      if (context.abortPreviousId) {
        const controller = new AbortController();

        operation.setContext({
          ...context,
          controller,
          fetchOptions: {
            signal: controller.signal,
          },
        });

        Object.values(inFlightOperations).forEach(operationData => {
          if (operationData?.abortPreviousId === context.abortPreviousId) {
            // If a controller exists, that means this operation should be aborted.
            operationData?.controller?.abort();
          }
        });

        inFlightOperations[requestId] = {
          controller: controller,
          abortPreviousId: context.abortPreviousId,
        };
      }

      return () => {
        // We don't need to keep around the requests, remove them once we are finished.
        delete inFlightOperations[requestId];

        if (handle) {
          handle.unsubscribe();
        }
      };
    })
);
  1. Then, you just need to pass in some unique identifier for your component. I have a wrapped useQuery function so I don't need to create the ID myself in every component, and rather, I just specify "abortPrevious" as true from components. But this works as well:
const Foo = () => {
  const abortPreviousId = useRef(uuid());
  const { data, loading, error } = useQuery(SomeQuery, {
    context: {
      // Make sure we only abort queries from this particular component, another
      // component might be using the same query simultaneously and doesn't want
      // to be aborted.
      abortPreviousId: abortPreviousId.current,
    },
  });
  return <p>bar</p>;
};
dylanwulf commented 4 years ago

@dannycochran Thank you for sharing!

guilhermeKodama commented 4 years ago

Any news on this one. It seems there are a lot of hacky ways to do it but any oficial PR?

blocka commented 4 years ago

@dannycochran what is the request function?

memark commented 4 years ago

This is needed!

klis87 commented 4 years ago

Because more than 2 years passed since raising this issue, if anyone is interested, I developed a library similar to Apollo - redux-requests, but for any API - not only GraphQL - with native and automatic abort support. It has also other features, like caching, optimistic updates, automatic normalisation, Redux level SSR and so on. For people who care about requests aborts, I recommend to check it out here on github. Don't be scared with Redux, as in practice you need to write just actions and use built-in selectors.

nirus commented 4 years ago

(Solved)

After struggling for day i was finally able to get the solution. People who are looking for the answers to cancel the pending request, i have documented the POC code & solution walkthrough step by step in this stackoverflow post (Read along.)

Thanks to this discussion thread. It helped a lot to arrive at the solution.

Credits to @dannycochran & @bgultekin solutions in this thread.

Hope this helps someone out there.

julkue commented 4 years ago

I also really need this to have the possibility to mark several filters as checked and only take continue with the latest requrest but cancel any earlier requests. While I was able to .unsubscribe() on a watchQuery to make sure my code isn't execute redundantly (see e.g. https://github.com/apollographql/apollo-client/issues/4150#issuecomment-500127694) the requests will still be handled by the browser. What I really need is a way to cancel the request on a network layer side.

uncvrd commented 4 years ago

I would love to have a cancel method be exposed on an Apollo query. My use case is that I have a page with tabs, and depending on which tab is active, I load different data on the page. When the page loads, it automatically selects tab1 and starts querying using Apollo for data in my useEffect hook. However, if I pass logic to go straight to tab2 I received the error:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

This is because of the following flow:

  1. Page loads with tab1 as default.
  2. Query in useEffect on tab1 sends the request
  3. Using react-router-dom state variable I switch the tab state to tab2
  4. The request has not completed yet so I received the error described above
  5. tab2 is loaded on the page.

This problem would be resolved (I think?) by allowing cancel where I could use it within my useEffect() like so to clean up unfinished queries on component unmount:

useEffect(() => {
    if (user && user.id) {
        myQuery({
            variables: {
                userId: user.id
            },
        });
    }

    return () => {
        cancel()
    }
}, [user, myQuery]);
Covmon commented 4 years ago

Actually, the hack above can be improved: no need to store the internalSendingId in the query variables: taking advantage of closures and local variables scopes, it can be simplified to:

this.lastFilterQuerySent = Math.floor(Math.random() * 100000000);
const localQuerySentId = this.lastFilterQuerySent;

targetQuery.fetchMore({
      variables: {
        // ... other variables
      },
      updateQuery: (prev, result) => {
        if (
          !result.fetchMoreResult ||
          localQuerySentId !== this.lastFilterQuerySent
        ) {
          // discard since it's not the last query
          return prev;
        }
        // ok take into account
        return result.fetchMoreResult;
      },
    });

It still smells, but a bit less: caching strategies can apply normally because the variables are not touched (although it seems fetchMore calls are never cached as far as I can see in my testing examples).

Is there an adaptation of this solution @qortex that works for functional components? Would I use state instead of class variables?

jordanwilking commented 3 years ago

@nirus's solution seems to be working for my use case. If my query doesn't finish by a certain time, I want to fall back to pinging my database and refetch once we have connection again. The following code is still a WIP, but it seems to be working (note: follow his steps first):

const INITIAL_TIMEOUT = 9000
const EXTENDED_TIMEOUT = 5000
const PING_TIMEOUT = 2000
const PING_FREQ = 5000

const useTest = (query: DocumentNode, options?: QueryHookOptions) => {
  const [requestTrackerId] = useState(uuid())
  const [timeout, setTimeout] = useState(INITIAL_TIMEOUT)
  const [extendedTimeout, setExtendedTimeout] = useState(false)
  const [startPinging, setStartPinging] = useState(false)
  const netInfo = useNetInfo()
  const result = useQuery(query, {
    ...options,
    context: { requestTrackerId },
  })

  // Check connection to database
  const ping = async () => {
    console.log('Ping')
    try {
      await fetchWithTimeout(`${TEMP_SERVER_URL}ping`, {
        timeout: PING_TIMEOUT,
      })
      console.log('Ping success')
      result.refetch()
      setTimeout(INITIAL_TIMEOUT)
      setStartPinging(false)
      setExtendedTimeout(false)
    } catch (e) {
      console.log('Ping failed')
    }
  }

  // If our query hasn't finsihed by the timeout
  useTimeout(() => {
    // Still loading and we have a connection, so let's fall back to pinging the database
    if (extendedTimeout && result.loading && netInfo.isConnected) {
      // tell user to check their connection
      setStartPinging(true)
      setExtendedTimeout(false)
      console.log('Waited, but nothing happend')
      // Loading and we have a connection, so let's give it some more time
    } else if (result.loading && netInfo.isConnected) {
      // tell user that things are taking longer than expected
      setTimeout(EXTENDED_TIMEOUT)
      setExtendedTimeout(true)
      console.log('Extending timeout')
      // We're not connected, let's fall back to pinging the database
    } else if (!netInfo.isConnected) {
      // tell user to check their connection
      setStartPinging(true)
      setExtendedTimeout(false)
      console.log('Not connected')
    }
  }, timeout)

  useInterval(() => {
    // Ping until we start seeing a connection again
    if (startPinging) {
      ping()
    }
  }, PING_FREQ)

  return { ...result, delayed: extendedTimeout, disconnected: startPinging }
}
Saransh27 commented 3 years ago

Any progress on this? In our project we have a react component to upload multiple images, now we want to add image cancelation ability, I had a look at the source code, it seems that we can pass a fetchOptions using mutation function:

const { mutation: uploadImg } = uploadImageObject;
uploadImg({
   variables: {
     file
   },
   context: {
     fetchOptions: {
      signal,
     }
   }
});

But when I trigger the abort controller, nothing happens, it actually doesn't stop pending request. any idea?

anybody able to solve this issue of cancelling a mutation? I tried abort, client.stop etc but nothing seems to be working. We use apollo client v2.4.1. @SirwanAfifi

truesteps commented 3 years ago

Hey guys! Any update on this issue? Will it be implemented?

2xSamurai commented 3 years ago

+1

For some reason, I feel this is a very important issue. Still, it's not closed even after 3 years. I just started using apollo a few months back. Even I feel that we need something like cancel in the hooks API. So why is it still not implemented? Is there something technically hard for it to be implemented? I don't understand.

Thanks for your hard work.

dayrlism10 commented 3 years ago

+1

ShakurOo commented 3 years ago

+1

quangpl commented 3 years ago

Up ! This is really a critical issue. I hope this will be implemented as soon as.

digeomel commented 3 years ago

If anybody's using the latest Apollo Client (3.4.13) with Apollo Angular (2.6.0) and Angular 12, I managed to abort existing requests (queries) by using watchQuery instead of query and calling unsubscribe() on the previously returned subscription.

hwillson commented 3 years ago

Hi all - as mentioned in https://github.com/apollographql/apollo-feature-requests/issues/40#issuecomment-489487888 this functionality is already supported in Apollo Client. We like the hook cancel suggestion in https://github.com/apollographql/apollo-feature-requests/issues/40#issuecomment-564704980 and will be looking into this. Thanks for the great discussion!

joshjg commented 3 years ago

@hwillson Although the custom AbortController approach does cancel the requests, it also causes this issue: https://github.com/apollographql/apollo-client/issues/4150

In the discussion there, it was recommended not to take this approach: https://github.com/apollographql/apollo-client/issues/4150#issuecomment-487412557

The suggested workaround in that issue is to use watchQuery + unsubscribe - but that's not really feasible for our use case in which we want to cancel ALL pending requests and selectively refetch only a subset.

The only way to do this that I've found is an ugly hack, reaching into Apollo's internals like this:

client.queryManager.inFlightLinkObservables = new Map();
// Now refetch certain queries

Edit: related question, should calling refetch multiple times in a row, before the previous request is complete, cancel the previous request? Currently, subsequent refetch calls seem to do nothing - they simply return the in-flight Promise.

digeomel commented 3 years ago

@joshjg I don't understand why you can't kill ALL requests using watchQuery + unsubscribe. Why can't you create a service to store all your active subscriptions and unsubscribe them there?

joshjg commented 2 years ago

@digeomel We have a large app using Apollo's react hooks extensively - we aren't often calling client.query directly where it could be swapped out easily.

digeomel commented 2 years ago

@joshjg apologies, it wasn't clear to me that you're using React. However, I suspect that your "ugly hack" solution can result in memory leaks, as it seems that your existing subscriptions are not properly unsubscribed.

FreTimmerman commented 2 years ago

@nirus

(Solved)

After struggling for day i was finally able to get the solution. People who are looking for the answers to cancel the pending request, i have documented the POC code & solution walkthrough step by step in this blog post (Read along.)

Thanks to this discussion thread. It helped a lot to arrive at the solution.

Credits to @dannycochran & @bgultekin solutions in this thread.

Hope this helps someone out there.

This is great. but do i understand correctly that this only cancels a request when a new request with the same id gets made? Can i cancel a request without having to send a new one?

flippidippi commented 2 years ago

Is this issue just for manually aborting requests?

We are having an issue where we are using useQuery, the variables are changing, but the previous request that is not complete is not being canceled.

Note, it does seem to work on component unmounting though.

levrik commented 2 years ago

@flippidippi We're having this exact same issue. I thought this issue is about implementing this and not about manual cancellation but you seem to be right.

Is there a tracking issue about aborting requests when variables are changing?

flippidippi commented 2 years ago

@levrik I'm not sure? Which is why I was asking if this issue covered this. In the original issue, it says

cancelling previous request if there is new pending

which seems like that should be about when variables change, but most of the comments under here seem like they are addressing the more manual approach.

@hwillson should this be a separate issue?

levrik commented 2 years ago

@flippidippi Sorry. Question about if an issue already exists wasn't directed to you, thus the line break 😅

gmavritsakis commented 2 years ago

Hi everyone. I am using "@apollo/client": "^3.5.6" and nothing worked properly for me. Also, I am using @graphql-codegen and I don't have access to the underlying observables.

So, after gathering the information from various sources I came up with the following solution which seems to be working good for my case. I used the base structure from here but changed the way it works so as to remove the AbortController which was creating problems (the cancelationLink was not working properly after the first cancellation, here is explained why).

Actually, what I do, is store the previous observer and throw an error to it which cancels that previous request when a new request comes up for the same operation.

This behavior gets applied automatically to all calls. If the user wants to prevent it for some calls he will have to pass a preventCancelations: true option inside the context.

I am not 100% sure that it does not create some kind of memory leaks or other subscription based issues, but I have been using this for some time now, without any obvious issues.

If anyone has the time please verify.


import { ApolloLink, Observable } from '@apollo/client';

const connections: { [key: string]: any } = {};

export const cancelRequestLink = new ApolloLink(
    (operation, forward) =>
        new Observable(observer => {
            const context = operation.getContext();
            const { operationName } = operation;

            const connectionHandle = forward(operation).subscribe({
                next: (...arg) => observer.next(...arg),
                error: (...arg) => {
                    delete connections[operationName];
                    observer.error(...arg);
                },
                complete: (...arg) => {
                    delete connections[operationName];
                    observer.complete(...arg);
                }
            });

            if (!context.preventCancelations) {
                if (connections[operationName]) {
                    connections[operationName].error(new Error('Request aborted'));
                    delete connections[operationName];
                } 

                connections[operationName] = observer;
            }
            return connectionHandle;
        })
);