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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change to an activity based timeout #1289

Merged
merged 3 commits into from
Oct 15, 2021
Merged

Change to an activity based timeout #1289

merged 3 commits into from
Oct 15, 2021

Conversation

Tratcher
Copy link
Member

Fixes #1052
Fixes #118 (comment)

The request Timeout used to cover the time until the response headers were received, which may also include sending the request body. This timeout could be problematic for large requests or gRPC client streaming. There was no timeout on the response body or on WebSockets which risked a resource leak.

Now a single activity token and timeout is used to track 'liveness', where that timeout is reset any time there's observable progress such as:

  • Response headers received
  • Request body data received
  • Request body data sent
  • Response body data received
  • Response body data sent
  • WebSocket data sent or received, including pings.

What doesn't count:

  • HTTP/2 pings
  • TCP keep-alives

This way we're not limiting the size of a request nor the direction of the traffic, so long as there continues to be any traffic.

Server request and response body data rates may still apply (though we opt out of those for gRPC & Kestrel).

(Thanks to @MihaZupan's last change, this was pretty easy 馃榿, with many of the changes being aesthetic.)

@Tratcher Tratcher added this to the YARP RC1 milestone Oct 14, 2021
@Tratcher Tratcher self-assigned this Oct 14, 2021
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Nice, I like how non-invasive this is.

A perf note: We'll want to test what impact this has when performing many reads/writes eventually.
For example, we could avoid resetting the timer each time by allowing a 1 second grace period.

src/ReverseProxy/Forwarder/StreamCopyHttpContent.cs Outdated Show resolved Hide resolved
@Tratcher Tratcher enabled auto-merge (squash) October 15, 2021 15:14
@Tratcher
Copy link
Member Author

I'll keep an eye on the next benchmark run to see what the impact is.

@Tratcher Tratcher merged commit 2015d99 into main Oct 15, 2021
@Tratcher Tratcher deleted the tratcher/activity branch October 15, 2021 16:02
@Kahbazi
Copy link
Collaborator

Kahbazi commented Oct 15, 2021

@Tratcher breaking-change label?

@Tratcher
Copy link
Member Author

@Tratcher breaking-change label?

Yes, thanks for the reminder.

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