Skip to content

Commit 36eb2cd

Browse files
committed
Fix concurrency bug in results view
In the results view, `setState` was used to set some state, and then `loadResults` was called to set some other state. However, `setState` is asynchronous, so the `this.state` in `loadResults` was not the state that was set before the call. This commit fixes it by combining the two `setState` calls into one. The logic was hard to follow, so I'm not sure if this is the correct fix. The `shouldKeepOldResultsWhileRendering` state seems to be unnecessary now since everything is being done in `setState` call, but I'm not sure about that.
1 parent 605315c commit 36eb2cd

1 file changed

Lines changed: 52 additions & 67 deletions

File tree

extensions/ql-vscode/src/view/results/results.tsx

Lines changed: 52 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@ export class ResultsApp extends React.Component<
104104
queryPath: msg.queryPath,
105105
});
106106

107-
void this.loadResults();
108107
break;
109108
case "showInterpretedPage": {
110109
const tableName =
@@ -141,7 +140,6 @@ export class ResultsApp extends React.Component<
141140
queryName: msg.queryName,
142141
queryPath: msg.queryPath,
143142
});
144-
void this.loadResults();
145143
break;
146144
}
147145
case "resultsUpdating":
@@ -164,76 +162,52 @@ export class ResultsApp extends React.Component<
164162

165163
private updateStateWithNewResultsInfo(resultsInfo: ResultsInfo): void {
166164
this.setState((prevState) => {
167-
const stateWithDisplayedResults = (
168-
displayedResults: ResultsState,
169-
): ResultsViewState => ({
170-
displayedResults,
171-
isExpectingResultsUpdate: prevState.isExpectingResultsUpdate,
172-
nextResultsInfo: resultsInfo,
173-
});
174-
175-
if (!prevState.isExpectingResultsUpdate && resultsInfo === null) {
176-
// No results to display
177-
return stateWithDisplayedResults({
178-
resultsInfo: null,
179-
results: null,
180-
errorMessage: "No results to display",
181-
});
182-
}
183-
if (!resultsInfo || !resultsInfo.shouldKeepOldResultsWhileRendering) {
184-
// Display loading message
185-
return stateWithDisplayedResults({
186-
resultsInfo: null,
187-
results: null,
188-
errorMessage: "Loading results…",
189-
});
165+
if (resultsInfo === null) {
166+
if (prevState.isExpectingResultsUpdate) {
167+
// Display loading message
168+
return {
169+
displayedResults: {
170+
resultsInfo: null,
171+
results: null,
172+
errorMessage: "Loading results…",
173+
},
174+
isExpectingResultsUpdate: prevState.isExpectingResultsUpdate,
175+
nextResultsInfo: resultsInfo,
176+
};
177+
} else {
178+
// No results to display
179+
return {
180+
displayedResults: {
181+
resultsInfo: null,
182+
results: null,
183+
errorMessage: "No results to display",
184+
},
185+
isExpectingResultsUpdate: prevState.isExpectingResultsUpdate,
186+
nextResultsInfo: resultsInfo,
187+
};
188+
}
190189
}
191-
return stateWithDisplayedResults(prevState.displayedResults);
192-
});
193-
}
194-
195-
private getResultSets(resultsInfo: ResultsInfo): readonly ResultSet[] {
196-
const parsedResultSets = resultsInfo.parsedResultSets;
197-
const resultSet = parsedResultSets.resultSet;
198-
if (!resultSet.t) {
199-
throw new Error(
200-
'Missing result set type. Should be either "InterpretedResultSet" or "RawResultSet".',
201-
);
202-
}
203-
return [resultSet];
204-
}
205190

206-
private async loadResults(): Promise<void> {
207-
const resultsInfo = this.state.nextResultsInfo;
208-
if (resultsInfo === null) {
209-
return;
210-
}
191+
let results: Results | null = null;
192+
let statusText = "";
193+
try {
194+
const resultSets = this.getResultSets(resultsInfo);
195+
results = {
196+
resultSets,
197+
database: resultsInfo.database,
198+
sortStates: this.getSortStates(resultsInfo),
199+
};
200+
} catch (e) {
201+
let errorMessage: string;
202+
if (e instanceof Error) {
203+
errorMessage = e.message;
204+
} else {
205+
errorMessage = "Unknown error";
206+
}
211207

212-
let results: Results | null = null;
213-
let statusText = "";
214-
try {
215-
const resultSets = this.getResultSets(resultsInfo);
216-
results = {
217-
resultSets,
218-
database: resultsInfo.database,
219-
sortStates: this.getSortStates(resultsInfo),
220-
};
221-
} catch (e) {
222-
let errorMessage: string;
223-
if (e instanceof Error) {
224-
errorMessage = e.message;
225-
} else {
226-
errorMessage = "Unknown error";
208+
statusText = `Error loading results: ${errorMessage}`;
227209
}
228210

229-
statusText = `Error loading results: ${errorMessage}`;
230-
}
231-
232-
this.setState((prevState) => {
233-
// Only set state if this results info is still current.
234-
if (resultsInfo !== prevState.nextResultsInfo) {
235-
return null;
236-
}
237211
return {
238212
displayedResults: {
239213
resultsInfo,
@@ -246,6 +220,17 @@ export class ResultsApp extends React.Component<
246220
});
247221
}
248222

223+
private getResultSets(resultsInfo: ResultsInfo): readonly ResultSet[] {
224+
const parsedResultSets = resultsInfo.parsedResultSets;
225+
const resultSet = parsedResultSets.resultSet;
226+
if (!resultSet.t) {
227+
throw new Error(
228+
'Missing result set type. Should be either "InterpretedResultSet" or "RawResultSet".',
229+
);
230+
}
231+
return [resultSet];
232+
}
233+
249234
private getSortStates(
250235
resultsInfo: ResultsInfo,
251236
): Map<string, RawResultsSortState> {

0 commit comments

Comments
 (0)