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: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking #3495

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 23, 2024

superseeds #3494
superseeds #3418
fixes #3493
fixes #3410

  • an internal "clock" is now used instead of Date.now() - ensuring that changes of the system clock wont effect the timers
  • timers.js exports a now method to retrieve the value of the internal clock
  • fasttimers clearTimeout expects a FastTimer instance or a native Timer. Before it accepted also undefined or null values
  • connect timeouts are now using fasttimers

Based on the work of @ronag i refactored the fast timers and hopefully fixed also the connection timeout bug when the event loop is blocked.

@Uzlopak Uzlopak force-pushed the refactor-fast-timers branch from cf4ca88 to ea4e9e8 Compare August 23, 2024 15:07
@Uzlopak Uzlopak marked this pull request as ready for review August 23, 2024 16:38
@Uzlopak Uzlopak changed the title chore: refactor fast timers fix: refactor fast timers Aug 23, 2024
@Uzlopak Uzlopak changed the title fix: refactor fast timers fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking Aug 23, 2024
@Uzlopak Uzlopak force-pushed the refactor-fast-timers branch from d87f441 to ac7a6e0 Compare August 23, 2024 20:59
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

This is a huge PR... not sure I have time to review properly in forseeable future.

@thisislvca
Copy link

This is a big issue over at Vercel, know it's a big PR but it's also quite important for lots of people :)

@ronag
Copy link
Member

ronag commented Aug 24, 2024

There are simpler PRs that solve the issue. I think the problem here is that we are combining a significant refactoring with bug fixes.

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

can you run the benchmark before/after to see if this impacted us at all?

@mcollina
Copy link
Member

After a quick skim it seems fine, and all tests are passing. If it introduces some regresisons, then I'm pretty sure some users of Next.js/Vercel would report this quickly find out.

@ronag
Copy link
Member

ronag commented Aug 24, 2024

I'm +-0

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 24, 2024

I want to add a small explaination of this refactoring.

I wanted to modify the fasttimer implementation. Yes it is kind of simple while you are implementing it and everything makes sense. But when working on it the first time, it is kind of hard to grok. So I started to comment the code and e.g. also replace the magic numbers with constants, and renamed the atrtibutes to be more expressive.

While I was investigating how the native timers are structured, I realized I should use their data structure/naming of the attributes. Thats why I renamed e.g. delay with _idleTimeout or callback with _onTimeout.

image

Also I tried to rename variables. when a different name was making more sense. E.g. in client-h1.js value to delay.

So in my opinion to review the timers.js file the first step is to just read the comments and check if it matches with the code it describes. And if it is good commented you should have a good idea on how the fast timers actually work.

The rest should be then easy to review.

@ronag
Copy link
Member

ronag commented Aug 24, 2024

IMHO all the comments and explanations make it harder to grasp and read. But if everyone else is happy with it I won't block. Good job.

@ronag
Copy link
Member

ronag commented Aug 24, 2024

Could you maybe summarize what behavior changes this introduces?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 24, 2024

Of course. I will in about 30 min, when i am back from groceries shopping ;)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 24, 2024

Well, I was looking into the code while writing what changes I implemented. I made a coverage check and realized, that the changes in client-h1.js seem to be not covered by a unit test. So I am currently implementing a unit test for that, just as a sanity check.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 24, 2024

I reverted the changes regarding timeoutDeadline in client-h1,js as those changes are not covered by a unit test. As soon as I find a test case and those lines fix the test then I would add them back.

@thisislvca
Copy link

Is there a chance to push out the Vercel-related errror fix first (so the most urgent) and then open a separate PR for everything else?

@Uzlopak Uzlopak force-pushed the refactor-fast-timers branch from dd96443 to 33fe101 Compare August 25, 2024 15:00
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.

lgtm

@Uzlopak Uzlopak merged commit 8a09d77 into nodejs:main Aug 27, 2024
32 checks passed
@Uzlopak Uzlopak deleted the refactor-fast-timers branch August 27, 2024 15:07
@trivikr
Copy link
Member

trivikr commented Oct 2, 2024

Should this be backported to 6.x?

A backport was requested in follow-up PR, but it failed because of merge conflicts #3552 (comment)

Copy link
Contributor

github-actions bot commented Oct 3, 2024

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

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-3495-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8a09d77037024b80e4efc6df53bf8cd1d36b14eb
# Push it to GitHub
git push --set-upstream origin backport-3495-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 base branch is v6.x and the compare/head branch is backport-3495-to-v6.x.

@metcoder95
Copy link
Member

I believe it should, but will need manual backport

cc: @Uzlopak @mcollina

@trivikr
Copy link
Member

trivikr commented Oct 3, 2024

The backport fails to 6.x, as reducing memory usage in client-h1 from #3510 is not backported.

github-actions bot pushed a commit that referenced this pull request Oct 3, 2024
…blocking (#3495)

* fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking

* also test native timers

* improve the commentary of fasttimers

* revert changes in client-h1.js

* make fasttimers independent from performance.now

* less flaky?

(cherry picked from commit 8a09d77)
mcollina pushed a commit that referenced this pull request Oct 4, 2024
…blocking (#3495) (#3673)

* fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking

* also test native timers

* improve the commentary of fasttimers

* revert changes in client-h1.js

* make fasttimers independent from performance.now

* less flaky?

(cherry picked from commit 8a09d77)

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
@mcollina
Copy link
Member

mcollina commented Oct 4, 2024

@trivikr if you want to open a Pr to backport those commits down, please be our guest!

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 4, 2024

@mcollina

Thanks to the comment by @trivikr I understand now why the backporting failed. Anyhow, I think only #3675 needs to be merged?!

@trivikr
Copy link
Member

trivikr commented Oct 4, 2024

if you want to open a Pr to backport those commits down, please be our guest!

I followed the instructions posted by the bot in #3495 (comment), and then found out backporting failed as the previous PR wasn't backported. I thus posted #3495 (comment) to inform maintainers to take appropriate actions.

I'll provide console output in future, so that there's clarification about the comment.

@trivikr
Copy link
Member

trivikr commented Oct 4, 2024

Thanks to the comment by @trivikr I understand now why the backporting failed

You're welcome! And thank you for adding labels to the PRs in appropriate order.
I'll provide console output after following bot instructions in future, so that there's clarification on why the comment was made.

Anyhow, I think only #3675 needs to be merged?!

Yup. And then we can release new undici version (say https://github.com/nodejs/undici/releases/tag/v6.20.0) and merge it before next Node.js release (is it https://github.com/nodejs/node/releases/tag/v22.10.0)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants