Skip to content

lib: bypass delay in AbortSignal.timeout #58594

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Jun 6, 2025

Fixes: #58592

setTimeout should check for if delay is valid

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 6, 2025
@himself65 himself65 force-pushed the himself65/2025/06/06/timer branch from 7bc677a to 351dacb Compare June 6, 2025 02:55
@himself65 himself65 force-pushed the himself65/2025/06/06/timer branch from 351dacb to 83f5055 Compare June 6, 2025 03:10
Copy link

codecov bot commented Jun 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (02a1505) to head (83f5055).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58594      +/-   ##
==========================================
- Coverage   90.22%   90.21%   -0.02%     
==========================================
  Files         635      635              
  Lines      187513   187511       -2     
  Branches    36840    36813      -27     
==========================================
- Hits       169176   169154      -22     
- Misses      11106    11134      +28     
+ Partials     7231     7223       -8     
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 98.26% <ø> (-0.01%) ⬇️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@himself65 himself65 requested a review from jasnell June 7, 2025 06:57
@himself65 himself65 added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 7, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58594
✔  Done loading data for nodejs/node/pull/58594
----------------------------------- PR info ------------------------------------
Title      lib: bypass `delay` in AbortSignal.timeout (#58594)
Author     Alex Yang <[email protected]> (@himself65)
Branch     himself65:himself65/2025/06/06/timer -> nodejs:main
Labels     author ready, needs-ci
Commits    1
 - lib: bypass `delay` in AbortSignal.timeout
Committers 1
 - Alex Yang <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58594
Fixes: https://github.com/nodejs/node/issues/58592
Reviewed-By: Jake Yuesong Li <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58594
Fixes: https://github.com/nodejs/node/issues/58592
Reviewed-By: Jake Yuesong Li <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 06 Jun 2025 02:54:17 GMT
   ✔  Approvals: 1
   ✔  - Jake Yuesong Li (@jakecastelli): https://github.com/nodejs/node/pull/58594#pullrequestreview-2907023450
   ✘  This PR needs to wait 118 more hours to land (or 0 hours if there is one more approval)
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2025-06-08T02:12:11Z: https://ci.nodejs.org/job/node-test-pull-request/67324/
- Querying data for job/node-test-pull-request/67324/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/15514793432

@himself65 himself65 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 8, 2025
@panva panva removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 8, 2025
@panva
Copy link
Member

panva commented Jun 8, 2025

PRs need two reviews to be landable after 48 hours. With one review a 7 day wait period is observed. A passing CI is also required.

@nodejs-github-bot
Copy link
Collaborator

@panva
Copy link
Member

panva commented Jun 8, 2025

cc @nodejs/web-standards

Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

The delay argument should be validated per its WebIDL as unsigned long long, so the test vectors are incorrect.

const { convertToInt } = require('internal/webidl');

// ...

static timeout(delay) {
  const opts = { __proto__: null, enforceRange: true };
  delay = convertToInt('delay', delay, 64, opts);
  // ...
}

When done i'd suggest to change the PR title and commit message to

lib: validate AbortSignal.timeout delay per its WebIDL definition

You can also pull in the latest WPTs for AbortSignal with the following (requires https://github.com/nodejs/node-core-utils)

git node wpt dom/abort

and update test/wpt/status/dom/abort.json as such

{
  "timeout-shadowrealm.any.js": {
    "skip": "ShadowRealm support is not enabled"
  }
}

@panva panva added web-standards Issues and PRs related to Web APIs and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbortSignal.timeout throws RangeError with any decimal number input
4 participants