Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix node:timers abort callback reset #3380

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jan 21, 2025

Fixes #3364
Fixes #3363

@anonrig anonrig requested review from vicb and jasnell January 21, 2025 15:11
@anonrig anonrig requested review from a team as code owners January 21, 2025 15:11
@anonrig anonrig requested a review from hoodmane January 21, 2025 15:11
@jasnell
Copy link
Member

jasnell commented Jan 21, 2025

Can you add tests?

@vicb
Copy link
Contributor

vicb commented Jan 21, 2025

Can you add tests?

Agreed, see #3363

Note: I just updated my comments, you do need the let onCancel

@anonrig anonrig force-pushed the yagiz/fix-abort-cancel-timers-promises branch from 790032a to 529a313 Compare January 21, 2025 16:20
@anonrig
Copy link
Member Author

anonrig commented Jan 21, 2025

Can you add tests?

Added. This also closes #3363

@anonrig anonrig force-pushed the yagiz/fix-abort-cancel-timers-promises branch from 529a313 to c90fb66 Compare January 21, 2025 16:38
@anonrig anonrig enabled auto-merge (squash) January 21, 2025 16:39
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Thanks for adding test 🎉

@vicb
Copy link
Contributor

vicb commented Jan 21, 2025

There seems to be one legit test failure

@anonrig anonrig force-pushed the yagiz/fix-abort-cancel-timers-promises branch from c90fb66 to 3f0ffa1 Compare January 21, 2025 18:29
@vicb
Copy link
Contributor

vicb commented Jan 21, 2025

@anonrig IMO it's better to avoid force pushes after reviews so that it's easier to see what has changed

@anonrig
Copy link
Member Author

anonrig commented Jan 21, 2025

@anonrig IMO it's better to avoid force pushes after reviews so that it's easier to see what has changed

There was a git conflict.

@anonrig anonrig force-pushed the yagiz/fix-abort-cancel-timers-promises branch from 3f0ffa1 to bf740cf Compare January 21, 2025 20:37
@anonrig
Copy link
Member Author

anonrig commented Jan 21, 2025

I reverted one of the recommendations since it broke ASAN. Here's the correct test (as same as Node.js) that didn't cause ASAN errors:

    {
      const ac = new AbortController();
      const signal = ac.signal;
      const { promise, resolve } = Promise.withResolvers();
      rejects(timersPromises.setImmediate(10, { signal }), /AbortError/).then(
        resolve
      );
      ac.abort();
      await promise;
    }

@anonrig anonrig merged commit 6dd8a1a into main Jan 21, 2025
17 checks passed
@anonrig anonrig deleted the yagiz/fix-abort-cancel-timers-promises branch January 21, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants