reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.74k stars 1.17k forks source link

Invalidating a request mid-flight #2444

Open hornta opened 2 years ago

hornta commented 2 years ago

I have a need to invalidate a query request that is in mid-flight. At least that's what I believe I want to do but I will explain my use case.

So in our UI we have a simple checkbox/switch that a user can toggle on/off, essentially just toggling a boolean flag in a data model. We got a toggle mutation endpoint for this in which I've implemented the onQueryStarted to manually toggle the flag in our query endpoints(we got several endpoints for this data model but it doesn't matter).

Everything works good so far, there is no UI-delay what so ever, yay optimistic updates!!! :-)

However there is a problem when the user flicks the switch on/off in rapid succession. This triggers two mutations to our api, essentiallt taking out itself since it's just a flag that toggles from false -> true -> false. This is fine. But for the first flick(false -> true) the query endpoints gets invalidated and starts to refetch the data model since it's stale.

For the second flick (true -> false) we once more trigger a mutation to set the data model in the backend to false. The query endpoints hasn't responded yet but whatever they will return I consider it stale because I've mutated the data model AFTER the query endpoint fired.

This ends up in a weird state where the cached response in RTK differs from the state in my backend.

What are some solutions to this?

I was thinking of a way to "force invalidate" tags to tell RTK that even though it has mid-flight requests, they are stale. invalidateTags: [{ type: "Item", id: arg.id, forceInvalidate:true }]

Relates to: https://github.com/reduxjs/redux-toolkit/issues/2444

RED-12

AlexanderArvidsson commented 1 year ago

I just ran into this issue now. We have refetching enabled when focusing the window.

We have a button that mutates something in the backend, and we have optimistic UI to reflect this change instantly in the frontend. However, if the window was not in focus, and the user clicks the button (thus gaining focus), what happens is that the mutation starts and the UI changes accordingly, but the focus refetch pending request then resets it back to the original state, and then once the mutation is finished, we use invalidateTags to refetch the user, thus causing a flicker in the frontend.

This seems like a huge oversight in the optimistic UI, something that I know other libraries already handle.

I saw that React-Query handles this by cancelling pending queries during optimistic UI.

Can we get something similar? https://tanstack.com/query/v4/docs/guides/optimistic-updates?from=reactQueryV3&original=https://react-query-v3.tanstack.com/guides/optimistic-updates

useMutation({
  mutationFn: updateTodo,
  // When mutate is called:
  onMutate: async newTodo => {
    // Cancel any outgoing refetches
    // (so they don't overwrite our optimistic update)
    await queryClient.cancelQueries({ queryKey: ['todos', newTodo.id] })
    ...
brightsider commented 1 year ago

I need the same functionality. I want to use useQuery which can automatically cancel prev request instead of using useLazyQuery because with this we need to create more code.

rjgotten commented 8 months ago

Running into this as well. We have two mutations we need to separately do to different sever-side endpoints updating different aspects of a particular item, based on the same form data being posted. Both of these endpoints invalidate tags provided by the same query endpoint.

So when we run mutation A, followed by mutation B, we get a cache invalidation on the query endpoint corresponding to the state as of A, and because mutation B kicks in before the query can finish, we never actually get a second invalidation as there is already an in-flight refetch occuring.

Currently the only viable low-hanging workarounds are either to include arbitrary delays between mutations -- which is just plain awful; so yeah, not doing that -- or just not using declarative invalidation, but using api.util.invalidatesTags to do it manually, after the series of requests has finished.

Why does RTK not simply throw out the in-flight request and treat it as superceded? There's an abort infrastructure there, right?

Alternatively: Would it be feasible to have an API that can defer invalidation refetches until after having exited a critical section? That would probably be the most elegant solution to this particular version of the mid-flight invalidation problem.

markerikson commented 8 months ago

@rjgotten short answer is that aborting in-flight requests is a thing we never tried to design for. (Lenz also has some prior comments somewhere in the repo about "all requests getting cached anyway so why try to cancel anything?".)

I'm not sure what you mean by "exited a critical section here". I definitely understand the reference to mutexes, I'm just not sure what that would look like in the context of RTKQ.

I'd say we're definitely open to API proposals here, and one of my big goals for the year is to ship major improvements to RTKQ. Even better, if you'd be willing to take a shot at trying to put together a PR, that would give us something concrete to discuss and work with.

AlexanderArvidsson commented 8 months ago

Coming back to this issue almost 1.5 years later, we are still seeing issues around this. We have since turned off automatic refetching via window focus because it caused way too many issues than the small benefit we got (hint, we solved refetching much better with realtime pubsub updates).

However, while my initial comment was about refetching on focus, this happens a lot in other scenarios, the major one being: Let's say you have a re-orderable list, and the user can re-order this list by dragging. When they stop dragging, we want to save the new order to the backend. Let's also hypothetically say our API is horribly slow, like 2-3 seconds (which is not unrealistic for a high demand application). After the user has re-ordered the list, they instantly see the change locally. All is fine and the user is happy.

However, since the API is horribly slow, the user may re-order again before the 2-3 seconds request time. When they drag, it looks fine since optimistic UI kicks in and we send a new request to the backend. All is so far fine. But, when the first request comes back with the now outdated ordering, it's now applied to the order, and the items move back to what they were after the first drag action! After a little bit of time, the items are now re-ordered again to what the user finally decided the order should be.

This causes severe disruptions and confusions in the UI and this would be easily solvable by simply aborting/ignoring the currently pending fetches after we've initiated the second optimistic update. If there's a method of solving this that does not rely on aborting/ignoring responses (since we can't at the moment) and does not require manual transformations/ignores on the cache, let me know! My point still stands after 1.5 years, RTK-Query is probably one of the only popular libraries that currently does not allow aborting/ignoring requests, React-Query being the reference I sent previously.

I'm glad to hear a big focus for this year is to improve RTK-Query. I think it's a great library and I've been able to easily accomplish a lot of things with it! If I find time, I might tackle this since I have previously contributed to RTK-Query.

rjgotten commented 8 months ago

@markerikson I'm not sure what you mean by "exited a critical section here". I definitely understand the reference to mutexes, I'm just not sure what that would look like in the context of RTKQ.

E.g.

import { useMutationA, useMutationB, api } from "api-slice";

const [mutateA] = useMutationA();
const [mutateB] = useMutationB();

//...

api.util.invalidateTogether(async (transaction) => {
  await dispatch(mutateA({ /*... */ }, { transaction }).unwrap();
  await dispatch(mutateB({ /* ... */ },  { transaction }).unwrap();
});

Basically, something like this would collect all the invalidated tags raised into transaction and then process them after the async function has finished.

Cu3PO42 commented 6 months ago

I believe I have a similar use-case and would like to share my motivation:

My backend can push invalidations to the frontend. For the sake of simplicity, let's say there's a WebSocket connection over which a list of tags to be invalidated are sent.

Certain workflows trigger many invalidations over a short peried of time. This is something we want to improve and batch in the future, but for now I think this behavior is valid. I believe I am observing the following behavior:

I'm not familiar enough with the RTK infrastructure to be sure my understanding is accurate, but it seems plausible to me. In order to fix my use-case, I would need the option to mark query results as stale, even if the request is still in-flight. I do not necessarily need the results to be immediately discardad, I just need the query to run again.

I understand there is also support for streaming updates, but unfortunately the backend in this case is incapable of telling me the new, updated values, only that some data has changed and should be refetched via the regular endpoints.

rjgotten commented 6 months ago

@Cu3PO42 Sounds like your use case could be facilitated by having api.invalidateTags() take a (conceptual) force:true option that basically would tell RTK: no, I don't care about whatever is in-flight - abort it and refetch.

Actually- just making the part of the declarative API for tags invalidated by mutations take a similar optional parameter, would probably go a long ways towards fixing issues like this in general.

Cu3PO42 commented 6 months ago

Yes, I believe either of these would allow my usecase. To be honest, I had expected in-flight requests to be invalidated by default. Arguments can absolutely be made for the current behavior as well, but I think both are reasonable and should be achievable.

Cu3PO42 commented 6 months ago

While digging around the source code, trying to determine how to implement such behavior, I found the following option:

  /**
   * Defaults to `'immediately'`. This setting allows you to control when tags are invalidated after a mutation.
   *
   * - `'immediately'`: Queries are invalidated instantly after the mutation finished, even if they are running.
   *   If the query provides tags that were invalidated while it ran, it won't be re-fetched.
   * - `'delayed'`: Invalidation only happens after all queries and mutations are settled.
   *   This ensures that queries are always invalidated correctly and automatically "batches" invalidations of concurrent mutations.
   *   Note that if you constantly have some queries (or mutations) running, this can delay tag invalidations indefinitely.
   */
  invalidationBehavior?: 'delayed' | 'immediately'

This seems to fully cover my usecase. The version of RTK in the project is <2, so presumably we still are on the behavior of 'immediately'. This setting seems undocumented outside of the migration guide, however. Additionally, the docs say the default is immediately, wherease the migration guide and code set it to 'delayed'. I will open another issue about this when time permits.