Skip to content
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

Fixes race condition when changing back to initial variables before request finishes #11186

Merged
merged 9 commits into from
Sep 1, 2023
5 changes: 5 additions & 0 deletions .changeset/odd-beers-perform.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fixes an issue where triggering a query with differnt variables then rapidly changing back to the initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables.
jerelmiller marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 8 additions & 5 deletions src/core/QueryInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,14 @@ export class QueryInfo {
this.dirty = false;
}

getDiff(variables = this.variables): Cache.DiffResult<any> {
const options = this.getDiffOptions(variables);
getDiff(): Cache.DiffResult<any> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out there was only one area of the codebase that called getDiff with variables, which was the readCache function in QueryManager#fetchQueryByPolicy. That function calls queryInfo.init(...) before that readCache function is called, therefore updating this.variables before this function is executed.

This parameter was always confusing to me because it felt too easy to accidentally change the variables being looked at by this QueryInfo instance. I prefer more intentional changes to this.variables to try and reduce the surface area where variables could change.

There is still lots more that could be refactored to make it more intentional, but that feels outside the scope of this PR and could open a big can of worms. For now, this change seemed reasonable as a step toward that future without breaking existing functionality.

const options = this.getDiffOptions();

if (this.lastDiff && equal(options, this.lastDiff.options)) {
return this.lastDiff.diff;
}

this.updateWatch((this.variables = variables));
this.updateWatch(this.variables);

const oq = this.observableQuery;
if (oq && oq.options.fetchPolicy === "no-cache") {
Expand Down Expand Up @@ -460,8 +460,11 @@ export class QueryInfo {

// In case the QueryManager stops this QueryInfo before its
// results are delivered, it's important to avoid restarting the
// cache watch when markResult is called.
if (!this.stopped) {
// cache watch when markResult is called. We also avoid updating
// the watch if we are writing a result that doesn't match the current
// variables to avoid race conditions from broadcasting the wrong
// result.
if (!this.stopped && equal(this.variables, options.variables)) {
// Any time we're about to update this.diff, we need to make
// sure we've started watching the cache.
this.updateWatch(options.variables);
Expand Down
2 changes: 1 addition & 1 deletion src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ export class QueryManager<TStore> {
networkStatus,
});

const readCache = () => queryInfo.getDiff(variables);
const readCache = () => queryInfo.getDiff();
Copy link
Member Author

@jerelmiller jerelmiller Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As alluded to in #11186 (comment) this was the only area of the codebase that passed variables to getDiff. Since queryInfo.init(...) is called a few lines up, this argument felt unneeded since init will set this.variables.


const resultsFromCache = (
diff: Cache.DiffResult<TData>,
Expand Down
62 changes: 62 additions & 0 deletions src/core/__tests__/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
MockLink,
mockSingleLink,
subscribeAndCount,
wait,
} from "../../testing";
import mockQueryManager from "../../testing/core/mocking/mockQueryManager";
import mockWatchQuery from "../../testing/core/mocking/mockWatchQuery";
Expand Down Expand Up @@ -3199,3 +3200,64 @@ test("regression test for #10587", async () => {
);
expect(query1Spy.mock.calls).toEqual(finalExpectedCalls.query1);
});

// https://github.com/apollographql/apollo-client/issues/11184
test("handles changing variables in rapid succession before other request is completed", async () => {
interface UserCountQuery {
userCount: number;
}
interface UserCountVariables {
department: "HR" | null;
}

const query: TypedDocumentNode<UserCountQuery, UserCountVariables> = gql`
query UserCountQuery($department: Department) {
userCount(department: $department)
}
`;
const mocks = [
{
request: { query, variables: { department: null } },
result: { data: { userCount: 10 } },
},
{
request: { query, variables: { department: "HR" } },
result: { data: { userCount: 5 } },
delay: 50,
},
];

const client = new ApolloClient({
link: new MockLink(mocks),
cache: new InMemoryCache(),
});

const observable = client.watchQuery<UserCountQuery, UserCountVariables>({
query,
variables: { department: null },
});

observable.subscribe(jest.fn());

await waitFor(() => {
expect(observable.getCurrentResult(false)).toEqual({
data: { userCount: 10 },
loading: false,
networkStatus: NetworkStatus.ready,
});
});

observable.reobserve({ variables: { department: "HR" } });
await wait(10);
observable.reobserve({ variables: { department: null } });

// Wait for request to finish
await wait(50);

expect(observable.options.variables).toEqual({ department: null });
expect(observable.getCurrentResult(false)).toEqual({
data: { userCount: 10 },
loading: false,
networkStatus: NetworkStatus.ready,
});
});