Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion superset-frontend/src/components/Chart/chartAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,10 +407,12 @@ export function exploreJSON(
ownState,
) {
return async (dispatch, getState) => {
const state = getState();
const logStart = Logger.getTimestamp();
const controller = new AbortController();
const prevController = state.charts?.[key]?.queryController;
const queryTimeout =
timeout || getState().common.conf.SUPERSET_WEBSERVER_TIMEOUT;
timeout || state.common.conf.SUPERSET_WEBSERVER_TIMEOUT;

const requestParams = {
signal: controller.signal,
Expand All @@ -422,6 +424,14 @@ export function exploreJSON(
dispatch(updateDataMask(formData.slice_id, dataMask));
};
dispatch(chartUpdateStarted(controller, formData, key));
/**
* Abort in-flight requests after the new controller has been stored in
* state. Delaying ensures we do not mutate the Redux state between
* dispatches while still cancelling the previous request promptly.
*/
if (prevController) {
setTimeout(() => prevController.abort(), 0);
}
Comment on lines +432 to +434
Copy link
Member

Choose a reason for hiding this comment

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

Now we should not need setTimeout no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had not included the set timeout but tests fail. Tests fail without the setTimeout because abort() mutates the controller’s signal immediately, and Redux flags that as a mutation during dispatch. The timeout defers that mutation so Redux can complete the action.

Copy link
Member

@msyavuz msyavuz Nov 17, 2025

Choose a reason for hiding this comment

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

I guess the only other way is to write a custom middleware to do what you do here but i think this is fine and should cause no race conditions. Tagging @kgabryje for on opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this doesn’t introduce any race conditions ,it just defers the abort to the next tick, which is safe and expected IMO.

Copy link
Member

Choose a reason for hiding this comment

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

I think that works fine for now, but as a long term solution, Claude suggests not storing abort controllers in Redux at all, since they're inherently mutable. Let's keep this solution for now to push this fix forward


const chartDataRequest = getChartDataRequest({
setDataMask,
Expand Down
64 changes: 64 additions & 0 deletions superset-frontend/src/components/Chart/chartActions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import * as exploreUtils from 'src/explore/exploreUtils';
import * as actions from 'src/components/Chart/chartAction';
import * as asyncEvent from 'src/middleware/asyncEvent';
import { handleChartDataResponse } from 'src/components/Chart/chartAction';
import * as dataMaskActions from 'src/dataMask/actions';

import configureMockStore from 'redux-mock-store';
import thunk from 'redux-thunk';
Expand Down Expand Up @@ -110,6 +111,69 @@ describe('chart actions', () => {
.callsFake(data => Promise.resolve(data));
});

test('should defer abort of previous controller to avoid Redux state mutation', async () => {
jest.useFakeTimers();
const chartKey = 'defer_abort_test';
const formData = {
slice_id: 123,
datasource: 'table__1',
viz_type: 'table',
};
const oldController = new AbortController();
const abortSpy = jest.spyOn(oldController, 'abort');
const state = {
charts: {
[chartKey]: {
queryController: oldController,
},
},
common: {
conf: {
SUPERSET_WEBSERVER_TIMEOUT: 60,
},
},
};
const getState = jest.fn(() => state);
const dispatchMock = jest.fn();
const getChartDataRequestSpy = jest
.spyOn(actions, 'getChartDataRequest')
.mockResolvedValue({
response: { status: 200 },
json: { result: [] },
});
const handleChartDataResponseSpy = jest
.spyOn(actions, 'handleChartDataResponse')
.mockResolvedValue([]);
const updateDataMaskSpy = jest
.spyOn(dataMaskActions, 'updateDataMask')
.mockReturnValue({ type: 'UPDATE_DATA_MASK' });
const getQuerySettingsStub = sinon
.stub(exploreUtils, 'getQuerySettings')
.returns([false, () => {}]);

try {
const thunk = actions.exploreJSON(formData, false, undefined, chartKey);
const promise = thunk(dispatchMock, getState);

expect(abortSpy).not.toHaveBeenCalled();
expect(oldController.signal.aborted).toBe(false);

jest.runOnlyPendingTimers();

expect(abortSpy).toHaveBeenCalledTimes(1);
expect(oldController.signal.aborted).toBe(true);

await promise;
} finally {
getChartDataRequestSpy.mockRestore();
handleChartDataResponseSpy.mockRestore();
updateDataMaskSpy.mockRestore();
getQuerySettingsStub.restore();
abortSpy.mockRestore();
jest.useRealTimers();
}
});

afterEach(() => {
getExploreUrlStub.restore();
getChartDataUriStub.restore();
Expand Down
Loading