-
-
Notifications
You must be signed in to change notification settings - Fork 651
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please:
- Remove stylistic changes
- Add tests to cover it up
lib/dispatcher/client-h1.js
Outdated
@@ -60,7 +60,7 @@ const removeAllListeners = util.removeAllListeners | |||
|
|||
let extractBody | |||
|
|||
async function lazyllhttp () { |
There was a problem hiding this comment.
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?
Done! |
There was a problem hiding this 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?
Sure! The tests have been added under |
@mcollina I've added comments explaining why those checks are necessary. Let me know if anything needs clarification! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you fix the listing issues? |
@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! |
Linting seems to still fail for the testing files you added |
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 So I would actually recommend to require setTimeout fro node:timers. |
I still think we should require setTimeout from node:timers. |
…cific helper methods for timers, if refresh is not existing, create a new timeout for refreshTimeout
I modified it and I think it is now good to go. |
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
#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]>
This PR addresses two separate but related issues in environments where
setTimeout
/setFastTimeout
may return a numeric ID instead of a nativeTimeout
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
inParser#setTimeout
TypeError: fastNowTimeout.refresh is not a function
(and similarly.unref()
) inutil/timers.js
Changes include:
lib/dispatcher/client-h1.js
–Parser#setTimeout
this.timeout.unref()
in atypeof … === 'function'
guard.setTimeout
returns a numeric ID, we skip calling.unref()
and avoid the TypeError.lib/util/timers.js
–refreshTimeout
fastNowTimeout.refresh()
in atypeof … === 'function'
check.fastNowTimeout.unref()
when instantiating the timer.fastNowTimeout
is not a properTimeout
object.Why:
.unref()
/.refresh()
still work as before.Testing:
Parser#setTimeout
norutil/timers#refreshTimeout
throws.Status