Skip to content

fix: clear timeout before invoking the callback - #2082

Open
xianjianlf2 wants to merge 1 commit into
caolan:masterfrom
xianjianlf2:fix/timeout-callback-cleanup
Open

xianjianlf2 wants to merge 1 commit into
caolan:masterfrom
xianjianlf2:fix/timeout-callback-cleanup

Conversation

@xianjianlf2

Copy link
Copy Markdown

When a task completes before its timeout but the final callback throws, async.timeout never reaches clearTimeout. Even if the caller catches that exception, the timer fires later and invokes the callback a second time with ETIMEDOUT.

Clear the timer before invoking the final callback. The callback's exception still propagates unchanged. Add a regression test that catches the original exception and verifies that no later timeout callback arrives; it records two calls before the fix and one afterward.

Validation: npm test passes (lint and 691 Node tests). FirefoxHeadless passes all 679 browser tests.

AI assistance: implemented and tested with OpenAI Codex.

@lbesecker195 lbesecker195 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reproduced this on master. The task calls back in time and the final callback throws, so clearTimeout never runs. The caller catches the error, but 20ms later the same callback is called again with ETIMEDOUT. That happened both when the task called back synchronously and when it called back from a setTimeout.

With this branch, each of those cases gets exactly one call and the thrown error still reaches the caller. A task that finishes in time without throwing still gets its result, and a slow task still gets ETIMEDOUT.

timer is assigned before fn(...args) runs, so moving clearTimeout ahead of the callback also covers a task that calls back synchronously. The new test fails on master (expected 2 to equal 1) and passes here. npm test passes: 691 tests, up from 690 on master, and lint makes no changes.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants