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: Improve timeout handling during event loop lag #3418

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 24, 2024

Make timeouts behave more closer that what is expected during event loop lag.

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Make timeouts behave more closer that what is expected
during event loop lag.
Comment on lines +194 to +196
if (this.timeoutDeadline && this.timeoutDeadline < Date.now()) {
onParserTimeout(this)
}
Copy link
Member Author

@ronag ronag Jul 24, 2024

Choose a reason for hiding this comment

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

This will call Date.now() quite often... which is not optimal performance-wise. However, I don't see a smart way around it without using a worker as the point is to detect event loop lag before dispatching further events.

@mcollina
Copy link
Member

Have you got a reproduction of this? I'd like to tinker with this problem too.

@ronag
Copy link
Member Author

ronag commented Jul 24, 2024

Nope. Had an 8 hour drive last night and was thinking about it. We've had a few possibly related issues here and there.

@mcollina
Copy link
Member

Yes, I'm aware, I just don't know what's the best way to solve this.

@ronag
Copy link
Member Author

ronag commented Jul 24, 2024

Yes, I'm aware, I just don't know what's the best way to solve this.

What are your thoughts on my proposal in this PR?

@ronag
Copy link
Member Author

ronag commented Jul 24, 2024

I think we have 2 different problems:

  • For the connect, body and headers timeout: event loop lag can cause a false positive by including the lag in the timeout hence making the elapsed duration too long.
  • For the idle timeout: event loop lag can cause a false negative by delaying the timeout detection hence making the elapsed duration too short.

i.e. basically two problems which an inverse of eachother.

@metcoder95
Copy link
Member

Just out of curiosity, even with this, it might still fall under the problem of high event loop lag, isn't it? Or I get it wrong?

@ronag
Copy link
Member Author

ronag commented Jul 24, 2024

it might still fall under the problem of high event loop lag, isn't it? Or I get it wrong?

Not that I can think of?

@QuiiBz
Copy link

QuiiBz commented Aug 9, 2024

👋 Hi! Is there any way we can help/contribute to push this PR forward? We (Vercel) are getting more and more reports, which we believe are related to this specific issue.

I'm personally not super familiar with Undici's codebase, but please let me know if there is any way we can help.

@ronag
Copy link
Member Author

ronag commented Aug 9, 2024

Not high on my list. If you want more urgent help feel free to mail me.

@metcoder95
Copy link
Member

@ronag are we moving on this or do you need support on this front?

@ronag
Copy link
Member Author

ronag commented Aug 12, 2024

I don't have time to work on it ATM. Feel free to take over.

@metcoder95
Copy link
Member

In the issue @Uzlopak said he's working on it. If you need support or something, let me know, I can work on it later this week

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

Successfully merging this pull request may close these issues.

4 participants