-
Notifications
You must be signed in to change notification settings - Fork 563
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: use fasttimers for all connection timeouts #3552
Conversation
abc40ac
to
4922c97
Compare
Are the fast timers even needed anymore after nodejs/node#46579? |
Yea. I don't think that PR (nodejs/node#46579) makes much difference. |
Fast timers also adjust for event loop lag. |
Ok, then i change this PR to rip out fast timers |
4922c97
to
5dcfaaa
Compare
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.
Do not remove fast timers. That's the opposite of what I was trying to say.
5dcfaaa
to
7393ea3
Compare
Oh! good that i had a backup :). reverted |
but one thing maybe for interest The test run for ripped out fast timers passes the tests https://github.com/nodejs/undici/actions/runs/10714949877 This indicates that event loop lags are also handled properly by native timers. But I guess we want to keep our fast timers solution due to the fact, that it is still less resource hungry. |
IMO This shouldn't be true as it's based on the uv loop... to me it more indicates an issue with our test |
let me fix the request-timeout tests. |
Should we also use setFastTimeout here? Line 169 in 61a4b69
|
Yes |
adapted so far, but not all tests are reactivated. so please not merge yet |
…EOUT_KEEP_ALIVE (#3554) * fix: use native timeouts for TIMEOUT_IDLE, rename TIMEOUT_IDLE to TIMEOUT_KEEP_ALIVE * ensure that fast timers and native timers are set properly * . * .
Because you asked, why i change the port. The thing is, that I want to use the port for the connection timeout to have in the logs the chance to determine atleast which server/connection resulted in a connection timeout. |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3552-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 dca0aa0998cbdef28916b23d6300beb2fd979140
# Push it to GitHub
git push --set-upstream origin backport-3552-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x Then, create a pull request where the |
The backport fails to 6.x, as the refactor of fast timers in #3495 is not backported to 6.x |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3552-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 dca0aa0998cbdef28916b23d6300beb2fd979140
# Push it to GitHub
git push --set-upstream origin backport-3552-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x Then, create a pull request where the |
* fix: use fasttimers for all connection timeouts * Apply suggestions from code review * activate some tests * also use fastTimers in connect.js * fix tests * fix tests * fix tests * fix: use native timeouts for TIMEOUT_IDLE, rename TIMEOUT_IDLE to TIMEOUT_KEEP_ALIVE (#3554) * fix: use native timeouts for TIMEOUT_IDLE, rename TIMEOUT_IDLE to TIMEOUT_KEEP_ALIVE * ensure that fast timers and native timers are set properly * . * . * rename import * more informative connection error * ignore request-timeout binary file, rename clearAll to reset * fix * add test * use queueMicrotask earlier in the socket callbacks (cherry picked from commit dca0aa0)
* fix: use fasttimers for all connection timeouts * Apply suggestions from code review * activate some tests * also use fastTimers in connect.js * fix tests * fix tests * fix tests * fix: use native timeouts for TIMEOUT_IDLE, rename TIMEOUT_IDLE to TIMEOUT_KEEP_ALIVE (#3554) * fix: use native timeouts for TIMEOUT_IDLE, rename TIMEOUT_IDLE to TIMEOUT_KEEP_ALIVE * ensure that fast timers and native timers are set properly * . * . * rename import * more informative connection error * ignore request-timeout binary file, rename clearAll to reset * fix * add test * use queueMicrotask earlier in the socket callbacks (cherry picked from commit dca0aa0) Co-authored-by: Aras Abbasi <[email protected]>
Fixes #3410
Fixes #3553
currently WIP!
Assessment:
The PR #3495 was imho the right step to fix the event loop issues. But Connection issues were still reported. Maybe the issue still existing because timeouts smaller than 1000ms will be handled by native Timers. Maybe those have to handled by the low resolution fast timer logic too, so that event loop blockers are less effecting the processing of requests.
Maybe we should increase the resolution of the timers to 200ms?! It should be still more performant than having a gazillion of native timers?
Maybe we should change this else if to an if and maybe check if the timer missed a tick (with the side effect that we need to call
performance.now()
).undici/lib/util/timers.js
Line 159 in 89a46dd
So if we skipped a tick, we immediatly run the timer, because we are just too late?
But on the other hand, this could actually trigger timeouts too early?!
So yeah, I would prefer to increase the resolution.