codeant-ai-for-open-source[bot] commented on code in PR #42012:
URL: https://github.com/apache/superset/pull/42012#discussion_r3574771565
##########
superset-frontend/src/middleware/asyncEvent.test.ts:
##########
@@ -173,6 +173,83 @@ describe('asyncEvent middleware', () => {
expect(fetchMock.callHistory.calls(CACHED_DATA_ENDPOINT)).toHaveLength(1);
});
+ test('backs off exponentially when polling requests keep failing', async
() => {
+ jest.useFakeTimers();
+ try {
+ fetchMock.clearHistory().removeRoutes();
+ fetchMock.get(EVENTS_ENDPOINT, { status: 403 });
+ asyncEvent.init(config);
Review Comment:
Yes — that’s a real race.
Clearing `pollingTimeoutId` only cancels the **next scheduled timeout**. It
does not stop an already in-flight `fetchEvents()` call from resolving later
and running the rest of `loadEventsFromApi()`. Once that old invocation
resumes, it can still:
- update `consecutivePollingErrorCount`
- process events
- schedule a new timeout via the current `transport`
So on a polling→polling re-init you can end up with two polling loops, and
on a disabled re-init you can accidentally restart polling.
## Minimal fix
Add a module-level **generation / active token** that increments on
`init()`, and capture it inside `loadEventsFromApi()`. Before mutating state or
rescheduling, verify the token is still current.
Example shape:
```ts
let pollingGeneration = 0;
export const init = (appConfig?: AppConfig) => {
pollingGeneration += 1;
const currentGeneration = pollingGeneration;
if (pollingTimeoutId) clearTimeout(pollingTimeoutId);
if (!isFeatureEnabled(FeatureFlag.GlobalAsyncQueries)) return;
// ...
loadEventsFromApi(currentGeneration);
};
const loadEventsFromApi = async (generation: number) => {
if (generation !== pollingGeneration) return;
try {
const { result: events } = await fetchEvents(eventArgs);
if (generation !== pollingGeneration) return;
consecutivePollingErrorCount = 0;
if (events?.length) await processEvents(events);
} catch (err) {
if (generation !== pollingGeneration) return;
consecutivePollingErrorCount += 1;
logging.warn(err);
}
if (generation !== pollingGeneration) return;
if (transport === TRANSPORT_POLLING) {
pollingTimeoutId = window.setTimeout(
() => loadEventsFromApi(generation),
getPollingDelay(),
);
}
};
```
That guards the deferred callback path cleanly and prevents stale
invocations from reviving polling.
## On the regression tests
I agree it would be good to add a deferred-fetch regression test for both
cases:
1. **enabled → enabled re-init**: stale callback must not create a second
loop
2. **enabled → disabled re-init**: stale callback must not reschedule polling
This closes the race that the timeout-clear alone misses.
If you want, I can help with the rest of the PR comments too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
