Delayed tag invalidations

[GeorchW]

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:

1. The query Q starts.
2. Mutation M finishes. It invalidates the tag T. There is currently no query that provides the tag T, so no query is invalidated.
3. The query Q finishes. It provides the tag T.

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

• Does this change require an update to the documentation?
• Should the purpose of pendingTagInvalidations be documented somewhere in the code? If yes, where?
• I'm not sure whether I've put my code in the right places. Feel free to request changes :-)

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[netlify]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	712f90e
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/653d8da2ffe3f2000803623c
😎 Deploy Preview	https://deploy-preview-3116--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codesandbox-ci]

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:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration
rtk-esm-cra	Configuration

[Shrugsy]

Awesome work 👍

Regarding this behaviour:

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 #3105 you raised the following concern:

It could, however, cause problems for people using extremely long-running queries (since no tags are invalidated while they run), so it might make sense to allow disabling this behavior.

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.

• Query A has previously run, providing tag 'User'
• Query B starts, which would provide tag 'Post', but takes a long time to resolve (say 3 seconds after the following mutation)
• A mutation is fired & resolves, which intends to invalidate tags 'Post' & 'User'

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.

[GeorchW]

@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.

[phryneas]

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 :)

[Shrugsy]

@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.

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:

1. Invalidate tags immediately, refetching any 'settled' queries that provide those tags
2. Don't clear the "to be invalidated" tags yet, hold onto them
3. When the pending query resolves, if it provides any of the 'held' tags, refetch that query
4. Clear the "to be invalidated" tags

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

[phryneas]

@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. 🤔

[GeorchW]

@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:

• Pro: Long-running queries are not an issue any more
• Con: The implementation as a whole would be significantly more complicated
  • In particular, answering the question "has this query been invalidated by these tags" is something that neither the data structures nor the code is designed for right now -- we are asking "which queries have been invalidated by these tags" instead. We could just compute the entire list of invalidated queries and look if "our" query is in it
• Con: Mutation batching is currently "included" with barely any additional code, which would need a separate implementation (Document how to batch RTK Query mutations properly #2203)

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.

[GeorchW]

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. 🤔

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.

[GeorchW]

@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?

[phryneas]

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.

[markerikson]

Just to check: is this something we want to change as default behavior? or should it be opt-in?

[phryneas]

In 2.0, I could see this as a useful default behavior, yes.

[markerikson]

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.

[GeorchW]

Instead of flipping defaults back and forth, I'd suggest simply rebasing this onto 2.0 and release it a couple weeks later.

[dorsharonfuse]

Any update on this?
We badly need this...

[EskiMojo14]

Any update on this? We badly need this...

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.

i don't know how likely we are to release another 1.9 before 2.0, however - so this may end up being merged into master and then being worked into 2.0's codebase 🤔 that's not something i decide though.

edit: sounds like a decision was made to save this for 2.0, and then it never got rebased?

[GeorchW]

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?

[dorsharonfuse]

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.
I'd want to wait only for fetchX to finish.

Any chance of addressing this?
I think waiting for ALL queries to finish could be a lot to ask in some cases...

[GeorchW]

@dorsharonfuse

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.

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:

query A starts
query A ends, provides tag Y
query A starts again
mutation B starts
mutation B ends, invalidates tag X
query A ends, provides tag X

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.

I think waiting for ALL queries to finish could be a lot to ask in some cases...

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

[dorsharonfuse]

@GeorchW I get what you mean.
I guess I did kind of assume in my own implementation which tag is going to be provided, since it's a bit static in our case.

[markerikson]

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.

[markerikson]

❓ so the intent here is to delay invalidation until every outstanding request is done, not just ones that might be overlapping?

[GeorchW]

Yeah, we don't track it in that amount of detail. Maybe we should?

[GeorchW]

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.)

[markerikson]

❓ so this is configured at the full API level, not just per-endpoint?

[GeorchW]

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?

[markerikson]

✋ 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?

[GeorchW]

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.

[GeorchW]

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.)

[markerikson]

Okay. Rebased this against v2.0-integration. Updated a couple bits of test data that needed the new field included. Went ahead and flipped the default to "delayed".

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!

Addressed since then

[GeorchW]

Oh, thank you so much!

[mjwvb]

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?

1. Get all currently running queries when invalidateTags is called
2. Push/merge the tags into the pendingTagInvalidations in the state of each running query
3. After a query finished, check the pendingTagInvalidations and refetch accordingly

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.

[markerikson]

@mjwvb : when I said "per-endpoint", I was suggesting that the new invalidationBehavior field could be defined at both the createApi level and also at the build.query() level, in case there was more flexibility needed.

And yeah, @dorsharonfuse , could you share more details / example code of what you did?

[dorsharonfuse]

@markerikson What I basically did is utilize the Module system in RTK and implemented an extra module of my own where I overridden the invalidateTags action and the mutataions' invalidatesTags behavior like so:

export const pendingInvalidationsModuleName = Symbol('pendingInvalidationsModule');
export type PendingInvalidationsModule = typeof pendingInvalidationsModuleName;

declare module '@reduxjs/toolkit/query' {
	// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/naming-convention
	export interface ApiModules<BaseQuery extends BaseQueryFn, Definitions extends EndpointDefinitions, ReducerPath extends string, TagTypes extends string> {
		[pendingInvalidationsModuleName]: {
			util: {
				immediatelyInvalidateTags: ActionCreatorWithPayload<TagDescription<ETagType>[]>;
			};
		};
	}
}

export const pendingInvalidationsModule = (): Module<PendingInvalidationsModule> => ({
	name: pendingInvalidationsModuleName,
	init: (
		api
	): {
		injectEndpoint(endpointName: string, definition: EndpointDefinition<any, any, any, any>): void;
	} => {
		const anyApi = api as any as Api<BaseQueryFn, EndpointDefinitions, string, ETagType>;

		const { invalidateTags: immediatelyInvalidateTags, selectInvalidatedBy } = anyApi.util;

		// Override the `invalidateTags` action to push pending invalidations if needed
		const invalidateTags = createAction<PrepareAction<TagDescription<ETagType>[]>>(immediatelyInvalidateTags.type, (tags: TagDescription<ETagType>[]) => {
			const state = store.getState();
			const queriesToInvalidate = selectInvalidatedBy(state, tags);

			// If the tags that we're trying to invalidate have pending queries,
			// we need to wait for them to finish before we can invalidate the tags
			if (hasPendingQueries(state, queriesToInvalidate)) {
				store.dispatch(
					addPendingInvalidation({
						tags,
						endpoints: queriesToInvalidate.map(({ endpointName, originalArgs }) => ({ endpointName, args: originalArgs })),
					})
				);

				return { payload: [], meta: 'Pushed to pending invalidations' };
			}

			return immediatelyInvalidateTags(tags);
		});

		assign(api.util, { invalidateTags, immediatelyInvalidateTags });

		return {
			injectEndpoint(endpoint, definition): void {
				if (!isNil(definition.invalidatesTags)) {
					const endpointInvalidatesTags = definition.invalidatesTags;

					definition.invalidatesTags = (response, error, request, meta): readonly TagDescription<ETagType>[] => {
						let tagsToInvalidate: readonly TagDescription<ETagType>[];

						if (isFunction(endpointInvalidatesTags)) {
							tagsToInvalidate = endpointInvalidatesTags?.(response, error, request, meta) ?? [];
						} else {
							tagsToInvalidate = endpointInvalidatesTags ?? [];
						}

						const state = store.getState();
						const queriesToInvalidate = selectInvalidatedBy(state, tagsToInvalidate);

						// If the tags that we're trying to invalidate have pending queries,
						// we need to wait for them to finish before we can invalidate the tags
						if (hasPendingQueries(state, queriesToInvalidate)) {
							logger.debug('Pushing pending invalidations...', { queriesToInvalidate });

							store.dispatch(
								addPendingInvalidation({
									tags: tagsToInvalidate as TagDescription<ETagType>[],
									endpoints: queriesToInvalidate.map(({ endpointName, originalArgs }) => ({ endpointName, args: originalArgs })),
								})
							);

							return [];
						}

						return tagsToInvalidate;
					};
				}
			},
		};
	},
});

Then, I added another middleware that upon a fulfilled/rejected action goes over the pendingInvalidations array and checks if any of the tags should be invalidated like so:

export const pendingInvalidationsHandler: Middleware =
	({ dispatch, getState }) =>
	(next) => {
		return (action) => {
			const isQueryEnd = isFulfilled(action) || isRejected(action);
			const pendingInvalidations = selectPendingInvalidations(getState());

			if (isQueryEnd && pendingInvalidations.length) {
				const state = getState();

				// Find any pending invalidations that are relevant to the current query and don't have any more relevant queries that are pending.
				const [relevantInvalidations, otherInvalidations] = partition(pendingInvalidations, ({ endpoints }) => {
					const [relevantEndpoints, otherEndpoints] = partition(
						endpoints,
						({ endpointName, args }) => isEqual(action?.meta?.arg?.endpointName, endpointName) && isEqual(action?.meta?.arg?.originalArgs, args)
					);

					const isInvalidationRelevantForCurrentQuery = relevantEndpoints.length > 0;
					const hasPendingOtherQueries = hasPendingQueries(
						state,
						otherEndpoints.map(({ endpointName, args }) => ({ endpointName, originalArgs: args }))
					);

					return isInvalidationRelevantForCurrentQuery && !hasPendingOtherQueries;
				});

				if (relevantInvalidations.length) {
					const tagsToInvalidate = relevantInvalidations.reduce((result, { tags }) => [...result, ...tags], []);

					// Remove the relevant invalidations from the pending invalidations array
					dispatch(setPendingInvalidations(otherInvalidations));

					// If this is a successful request, maintain the current data in the store and don't update it with the new data
					// since a new request will be made soon anyway, and we don't want to show stale data in the UI.
					if (isFulfilled(action)) {
						const query = state?.api?.queries?.[action?.meta?.arg?.queryCacheKey];

						set(action, 'payload', query?.data);
					}

					// Let the fulfilled/rejected action finish running
					const result = next(action);

					// Invalidate the tags
					dispatch(immediatelyInvalidateTags(tagsToInvalidate));

					return result;
				}

				return next(action);
			}

			return next(action);
		};
	};

Note that immediatelyInvalidateTags is simply the regular invalidateTags from RTK.

Hope I haven't committed any sins by using this module, I haven't this kind of usage anywhere really.
For us, it works as it only waits for relevant queries to finish instead of all of them.

[GeorchW]

@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?

[GeorchW]

@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.

[minlare]

@GeorchW I'd like to see a mode that supports the above, as openapi-codegen is exactly the scenario I have.