-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Delayed tag invalidations #3116
Delayed tag invalidations #3116
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 712f90e:
|
Awesome work 👍 Regarding this behaviour:
In #3105 you raised the following concern:
I think that is a valid concern, and is a bit of a tradeoff rather than a strict upgrade. Let me know if I've misinterpreted any of the behaviour in the below example. e.g.
Here is where the behaviour diverges. Old behaviour: Query A is refetched. 3 seconds later, Query B resolves, but does not refetch. It has the potential to resolve with stale data. New behaviour: 3 seconds later, Query A is refetched, Query B resolves, then refetches. If it had stale data, it would be refetched with up to date data. i.e. under the new behaviour, refetching Query A is unnecessarily delayed until the other query resolves. Just something to consider. |
@Shrugsy Right, will add an option to disable it. Do you think it's fine to enable the new behavior by default? I think that would make sense, since it provides correctness of the query invalidations, and I would consider correctness to be more important than performance in edge cases, but I'm open to other arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general idea, but have some change suggestions.
Generally, I think we can slim this down a lot if yet-to-be-invalidated-tags are not saved to a separate slice, but just be kept in a per-store middleware-scoped variable.
I have added quite a lot of comments, I hope they make sense :)
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
Having a config option and two different versions seems like a bit much Ideally the behaviour would be roughly: When a mutation is performed while a query is still pending:
How feasible does it look to achieve step 3 - refetching a specific query if it provides the corresponding tags, WITHOUT doing so by merely invalidating the tags again? Noting that if it was done by invalidating tags again, that would cause an additional refetch of the settled queries |
@Shrugsy tbh., I think an ability to turn this on and off might be a good call since it significantly differs from current behavior. It might make sense to evaluate your suggestion, but I feel that the current approach has its merits. It might even be better to refetch all or none to minimize inconsistencies. The only negative thing I can see with the current approach is that on invalidation, things might not immediately go into a "pending" state, but wait for current pending queries/mutations to finish, so a loading indicator might appear late. 🤔 |
@Shrugsy I was thinking about this, too. We would need to store the "pending tag invalidations" per query then, instead of globally. I see the following pros and cons:
I would suggest keeping it simple and see how it's working out in practice, maybe adding a "Troubleshooting" entry that tells people to disable the new behavior if they have issues with long-running queries. If we get bug reports for which disabling the setting is not a viable workaround, we can still implement the "per-query" logic. |
Ugh, this didn't occur to me, good point. I think the most likely situation for the delay to trigger at all is just a couple milliseconds long, so this issue would be hard to observe. Again, I think it makes sense to see if this is an actual problem in practice before jumping to conclusions. |
@phryneas I've applied a few changes, want do do another round of review? Related: The implementation changed back and forth a bit due to the suggestions. Should I rebase to clean up the history? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This starts to look pretty good :)
I'm still a bit concerned about the need for a third, "combined" mode in the future, so let's directly pave the way for that.
No need to rewrite any commits, since we are going to squash that anyways, but please rebase this whole PR against the v2.0-integration branch.
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
Just to check: is this something we want to change as default behavior? or should it be opt-in? |
In 2.0, I could see this as a useful default behavior, yes. |
Okay, so maybe we ship this in 1.9.x with it defaulted to the existing behavior, and then flip the default in 2.0. |
Instead of flipping defaults back and forth, I'd suggest simply rebasing this onto 2.0 and release it a couple weeks later. |
bad ugly not good solution to the race condition on page load, but it works perfectly so w/e i'm delaying each query until the backend URL to query is specified the (imo) correct solution here is to correctly invalidate tags in the presence of a race condition. seems like there's a PR to do that here: reduxjs/redux-toolkit#3116 but no movement on it for the last few months related issue: reduxjs/redux-toolkit#3386
Any update on this? |
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
it's still marked as having changes requested by @phryneas, though by the looks of it all of those are resolved other than the nitpick i've added, everything else looks good to me.
edit: sounds like a decision was made to save this for 2.0, and then it never got rebased? |
Ah, I was under the impression that I just made the suggestion to rebase it and I never got a reply, but I see that the target milestone was changed in response. It doesn't break any APIs, so I hope it's fine to include it in the beta? I can rebase it in the next couple of days if this is the route we want to take. @phryneas @markerikson go/no-go? |
I'm wondering if there's any way to be more specific about which query we need to wait to finish before pushing out the pending tag invalidations. In my own implementation of this (since we're still waiting for this feature), we attached every pending tag invalidation to an endpoint name and args, and our middleware would compare the fulfilled/rejected action to see if it matches the endpoint name and args and only then would invalidate their specific attached tag . Meaning, if I want to invalidate fetchX once it's done running, I don't want to wait for all other running queries to finish, as they can easily be unrelated, and i'll just end up waiting longer for no good reason. Any chance of addressing this? |
I wondered that too, I don't think it's possible in general. I don't quite understand your approach. For example, you could have this situation:
How do you know that query A is going to provide tag X the next time? We can of course make the assumption for delayed tag invalidations that a query is going to provide the same tags as last time, but since this is not necessarily correct, I wouldn't make it the default.
Tbh, my gut feeling is that in most situations, you won't constantly have those long-running queries, and that the solution should be good enough. Queries usually finish in a couple hundred ms at most, and unrelated mutations are usually triggered by user interaction, which is further apart. The correctness issue is a bit more pressing IMO, so I'd like to get this merged soon-ish (whenever maintainers react), but if you have a good idea, I'm absolutely open to make another MR or support you with making one |
@GeorchW I get what you mean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, finally was able to take a meaningful look at this PR.
Good news is that it's a fairly small set of changes, which makes it easier to review and also suggests we're not radically changing things.
I do have some immediate questions / concerns:
- It looks like we're only able to set the invalidation behavior once for the entire API. This feels like something we might want to configure on a per-endpoint basis. How feasible is that?
- I think the current logic in the PR could end up with the same tags in
pendingTagInvalidations
multiple times, in which case I think we might try to process them multiple times as well - I'm not sure I understand why we're trying to wait for every pending query or mutation to complete before doing more work. At a guess: because any pending query could provide more tags, and any pending mutation could invalidate those tags?
The RTKQ side of things is admittedly not my strong point - I haven't worked on the code as much, and I don't have a deep understanding of data fetching needs the way Lenz does. But at first glance, something about this feels just a bit off, and I can't pinpoint exactly how. Like, I think I see the general intent of these changes, but I'm not immediately convinced this is the best answer to the batching/invalidation questions being raised. (I also don't have better suggestions atm either 🤷♂️ .)
@GeorchW : can you at least make a couple tweaks around ensuring we only process each tag once, and see if it even makes sense to specify the behavior per-endpoint? And we can discuss further from there.
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
const hasPendingRequests = [ | ||
...Object.values(state.queries), | ||
...Object.values(state.mutations), | ||
].some((x) => x?.status === QueryStatus.pending) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ so the intent here is to delay invalidation until every outstanding request is done, not just ones that might be overlapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't track it in that amount of detail. Maybe we should?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I just wrote it up. It's like 20 lines more, but it introduces quite a bit of complexity and its a bit harder to optimize. I think I'd advocate for keeping it simple and looking if people have a problem with it. (I suspect they won't.)
* 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ so this is configured at the full API level, not just per-endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I think this is a sane solution to a rare-ish case. For a use-case where one would note the delay, you would need to have an application where you have very long-running queries constantly, and mutations firing at the same time that they run. This tastes unhealthy for performance to begin with and I would strongly suggest the developer to fix the queries, but if that's not possible there's this escape hatch.
Or maybe I'm wrong. Do you have a particular use case in mind where a per-endpoint setting would have a strong advantage?
mwApi: SubMiddlewareApi | ||
) { | ||
const rootState = mwApi.getState() | ||
const state = rootState[reducerPath] | ||
|
||
pendingTagInvalidations.push(...newTags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ This looks like we could end up with the same tags showing up in pendingTagInvalidations
multiple times, which I think also means we'd end up running some extra logic when we try to invalidate. Can pendingTagInvalidations
be a Set
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since tags are objects and not strings in general. More specifically, their type is
{
type: TagType;
id?: string | number | undefined;
}
The set of invalidated endpoints is already deduplicated in selectInvalidatedBy
. Now, we could try de-duplicating here again by converting these tag objects into canonical string forms, but I think this is creating more overhead than it alleviates except in extreme cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and while its true that tags can end up in pendingTagInvalidations
multiple times, the amount of processing required is no different than if they were invalidated immediately. So this is not a performance regression. (It's not like this code will show up in the profiler anyway.)
ad4ad72
to
08667ca
Compare
Co-authored-by: Ben Durrant <ben.j.durrant@gmail.com>
08667ca
to
712f90e
Compare
Okay. Rebased this against I'm going to go ahead and merge this now, then put out another 2.0 beta with this included. Thank you for the change, and for your patience! |
Oh, thank you so much! |
Coming here from the 2.0.0-beta.4 releasenotes. Although a delayed solution works for most "perfect" server APIs, our API is far from perfect and has some queries that are taking longer than usual (out of my control unfortunately). A couple of seconds is not an exception for some of our endpoints, with extremes here and there. If I understand correctly we cannot use the delayed behavior and have to fallback to immediate. I don't know the internals, so only based on gut feeling: why not have a third behavior that does the following?
Is this what is meant by per-endpoint as stated above? Otherwise I'm curious about @dorsharonfuse's workaround, as it sounds exactly what I need. |
@mjwvb : when I said "per-endpoint", I was suggesting that the new And yeah, @dorsharonfuse , could you share more details / example code of what you did? |
@markerikson What I basically did is utilize the Module system in RTK and implemented an extra module of my own where I overridden the
Then, I added another middleware that upon a
Note that Hope I haven't committed any sins by using this module, I haven't this kind of usage anywhere really. |
@mjwvb TBH, it depends on the way your app works. The invalidation behavior only triggers when queries are running while mutations are applied. If your typical use case is like "load a bunch of data and allow the user to edit it, then save it when the user clicks 'save'", then it might be unlikely that queries are still running when the user clicks "save". But yeah, in general, you might suffer delays. @dorsharonfuse So, if I understand you correctly, you assume that the tags generated by a query are the same in each request? |
@dorsharonfuse ah right, I re-read the top part of the conversation again and we talked about this already a couple months back. Maybe we could "officially" support a mode that assumes this: queries always return the same tags for the same args, so we only need to delay invalidation for queries that provided now-invalidated tags last time they ran or that are running for the first time. I think this should not be the default behavior, but it could be useful for anyone with long-running queries and simple tags setup. E.g. with openapi-codegen, the assumption is guaranteed to be true. We'd need to make sure the documentation on this is clear, though. |
@GeorchW I'd like to see a mode that supports the above, as openapi-codegen is exactly the scenario I have. |
I've described the approach in this PR earlier in #3105.
Resolves #2203, #3105
Problem
There are race conditions when a mutation is finishing while a query is running. More concretely, this happens:
If the data from Q was generated before M arrived on the server, then the data is outdated. This is rare in normal circumstances, but when many mutations/queries are being started within a short time frame, it becomes likely.
Solution
If a mutation finishes while another query or mutation is running, don't invalidate tags. Instead, invalidate them later after all queries/mutations are settled.
In the much more common case that the mutation isn't running at the same time as a query, this won't change anything.
For the bug described above, it would be sufficient if the tag invalidation would only be delayed until all queries are settled (as opposed to queries + mutations). However, waiting for the mutations as well has the advantage of solving issue #2203 on the go: when multiple mutations are fired at the same time, queries will only start after all the mutations settled. In a way, they are batched automatically.
Open Questions
pendingTagInvalidations
be documented somewhere in the code? If yes, where?