Skip to content

Fix timer guards to avoid TypeError under fake‐timers and polyfilled … #4213

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.

Sign up for GitHub

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

Jump to bottom
Merged
mcollina merged 10 commits into from
Jun 26, 2025
Merged

Fix timer guards to avoid TypeError under fake‐timers and polyfilled … #4213

mcollina merged 10 commits into from
Jun 26, 2025

Conversation

@1ly4s0
Copy link
Contributor

@1ly4s0 1ly4s0 commented May 14, 2025 •
edited
Loading

This PR addresses two separate but related issues in environments where setTimeout/setFastTimeout may return a numeric ID instead of a native Timeout object (e.g., Jest modern fake timers, browser/Electron polyfills). Without these guards, calls to .unref() or .refresh() throw:

  • TypeError: this.timeout.unref is not a function in Parser#setTimeout
  • TypeError: fastNowTimeout.refresh is not a function (and similarly .unref()) in util/timers.js

Changes include:

  1. lib/dispatcher/client-h1.js – Parser#setTimeout

    • Wrapped the call to this.timeout.unref() in a typeof … === 'function' guard.
    • Ensures that if setTimeout returns a numeric ID, we skip calling .unref() and avoid the TypeError.

  2. lib/util/timers.js – refreshTimeout

    • Wrapped fastNowTimeout.refresh() in a typeof … === 'function' check.
    • Added a similar guard around fastNowTimeout.unref() when instantiating the timer.
    • Prevents errors when fastNowTimeout is not a proper Timeout object.

Why:

  • Improves compatibility with Jest’s modern fake timers and any runtime where Node.js timers are shimmed.
  • Prevents build/test failures caused by unguarded method calls on numeric timer IDs.
  • No behavioral changes in pure Node.js environments; native timers with .unref()/.refresh() still work as before.

Testing:

  • Added Jest tests under fake timers to confirm neither Parser#setTimeout nor util/timers#refreshTimeout throws.
  • Verified existing test suite passes unchanged.

Status

metcoder95
metcoder95 reviewed May 15, 2025
View reviewed changes
Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Can you please:

  1. Remove stylistic changes
  2. Add tests to cover it up

lib/dispatcher/client-h1.js

let extractBody

async function lazyllhttp () {
Copy link
Member

@metcoder95 metcoder95 May 15, 2025

Choose a reason for hiding this comment

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

Can you remove the styling changes?

@1ly4s0
Copy link
Contributor Author

1ly4s0 commented May 15, 2025

Can you please:

  1. Remove stylistic changes
  2. Add tests to cover it up

Done!

metcoder95
metcoder95 reviewed May 15, 2025
View reviewed changes
Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Can you add some tests for it?

@1ly4s0
Copy link
Contributor Author

1ly4s0 commented May 15, 2025 •
edited
Loading

Can you add some tests for it?

Sure! The tests have been added under test/jest/ (util-timers.test.js and parser-timeout.test.js), and npm run test:jest now passes with both new specs green. Let me know if there’s anything else you’d like adjusted!

@1ly4s0 1ly4s0 requested a review from metcoder95 May 15, 2025 19:20
mcollina
mcollina reviewed May 20, 2025
View reviewed changes
@1ly4s0
Copy link
Contributor Author

1ly4s0 commented May 20, 2025

@mcollina I've added comments explaining why those checks are necessary. Let me know if anything needs clarification!

mcollina
mcollina approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@metcoder95
Copy link
Member

metcoder95 commented May 20, 2025

Can you fix the listing issues?

@1ly4s0
Copy link
Contributor Author

1ly4s0 commented May 20, 2025

@metcoder95 If you were referring to the linting issues, they should be resolved with the latest commit. Let me know if there’s anything else!

@metcoder95
Copy link
Member

metcoder95 commented Jun 2, 2025

Linting seems to still fail for the testing files you added

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 2, 2025

I wonder... In deno setTimeout is returning a number because of spec compliance with the web spec. But if you want the node behavior, you should require the setTimeout from node:timers and it will have the right behaviour.

So I would actually recommend to require setTimeout fro node:timers.

Uzlopak
Uzlopak reviewed Jun 2, 2025
View reviewed changes
@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 26, 2025

I still think we should require setTimeout from node:timers.

Uzlopak added 3 commits June 26, 2025 12:56
@Uzlopak
Merge branch 'main' into 1ly4s0/main
3421efe
@Uzlopak
fix linting issue
f7acff5
@Uzlopak
use setTimeout from node:timers to increase chance of having node spe…
c42ed10 
…cific helper methods for timers, if refresh is not existing, create a new timeout for refreshTimeout
@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 26, 2025

I modified it and I think it is now good to go.

Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
Uzlopak
Uzlopak reviewed Jun 26, 2025
View reviewed changes
@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 26, 2025

@metcoder95
@mcollina

PTAL

mcollina
mcollina approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 995f064 into nodejs:main Jun 26, 2025
29 of 31 checks passed
@github-actions github-actions bot mentioned this pull request Jun 26, 2025
Merged
Uzlopak added a commit that referenced this pull request Jun 27, 2025
@1ly4s0 @Uzlopak
Fix timer guards to avoid TypeError under fake‐timers and polyfilled … (
24c2b9c 
#4213)

* Fix timer guards to avoid TypeError under fake‐timers and polyfilled environments

* Remove styling changes

* Add tests

* Add comments

* fix(lint): satisfy eslint stylistic rules for EOF, indent and jest globals

* Update lib/dispatcher/client-h1.js

* fix linting issue

* use setTimeout from node:timers to increase chance of having node specific helper methods for timers,  if refresh is not existing,  create a new timeout for refreshTimeout

* Apply suggestions from code review

---------

Co-authored-by: Aras Abbasi <[email protected]>
@ranisalt
Copy link

ranisalt commented Aug 13, 2025

Can this be backported to v6? We're facing this issue (with jest 29) but we're still on Node 22 while it's the latest LTS.

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

Reviewers

@Uzlopak Uzlopak Uzlopak left review comments

@metcoder95 metcoder95 metcoder95 approved these changes

@mcollina mcollina mcollina approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@1ly4s0 @metcoder95 @Uzlopak @ranisalt @mcollina