cherry-pick: fix(queue): waitForDrain poll fallback for drains that never emit (#1643)#1733
Conversation
) * fix(queue): poll fallback so waitForDrain cannot hang on a queue emptied without a drained emit * feat(table): expose evictionMS beside expirationMS * fix(queue): remove the drained listener when settle fires via the poll (review) * chore: format Table.ts with the pinned prettier --------- Co-authored-by: Kris Zyp <kriszyp@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a polling fallback mechanism in IterableEventQueue.ts to prevent waitForDrain from hanging when the queue empties without emitting a drained event, adds a static getter evictionMS to Table.ts, and includes a unit test for the new queue draining behavior. The reviewer identified a potential ReferenceError (Temporal Dead Zone) and memory leak in IterableEventQueue.ts if settle is executed synchronously before the poll interval is initialized, and provided a code suggestion to resolve this issue.
| const settle = (drained: boolean) => { | ||
| clearInterval(poll); | ||
| this.removeListener('drained', onDrained); | ||
| resolve(drained); | ||
| }; | ||
| const onDrained = () => settle(true); | ||
| this.once('drained', onDrained); | ||
| this.currentDrainResolver = settle; | ||
| const poll = setInterval(() => { | ||
| if (!this.queue || this.queue.length === 0) settle(true); | ||
| }, 100); | ||
| poll.unref?.(); |
There was a problem hiding this comment.
If settle is called synchronously (for example, if drained is emitted synchronously during this.once('drained', onDrained) or if close is emitted synchronously), calling clearInterval(poll) will throw a ReferenceError: Cannot access 'poll' before initialization due to the Temporal Dead Zone (TDZ) because poll is declared with const below settle.
Additionally, if settle runs synchronously, the interval poll will still be created afterwards and leak.
To fix this, we should declare poll with let at the top of the block, use a settled flag to avoid starting the interval if settle has already run, and clear this.currentDrainResolver to prevent memory leaks.
let poll: any;
let settled = false;
const settle = (drained: boolean) => {
settled = true;
if (poll) clearInterval(poll);
this.removeListener('drained', onDrained);
if (this.currentDrainResolver === settle) {
this.currentDrainResolver = null;
}
resolve(drained);
};
const onDrained = () => settle(true);
this.once('drained', onDrained);
this.currentDrainResolver = settle;
if (!settled) {
poll = setInterval(() => {
if (!this.queue || this.queue.length === 0) settle(true);
}, 100);
poll.unref?.();
}
|
Reviewed; no blockers found. |
2 participants
Manual redo of the automated cherry-pick (#1692), which left unresolved conflict markers from a stale branch point (pre-#1621/#1709/#1584).
Clean cherry-pick of #1643's single squash commit onto current
v5.1tip.Verified:
unitTests/resources/IterableEventQueue.test.js— 5/5 passing.Closes the stalled #1692.