Skip to content

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 requested a review from MihaZupan October 14, 2021 22:53
@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.

@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
Member

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

Development

Successfully merging this pull request may close these issues.

Security: Protect against slow-loris attacks via WebSockets & gRPC Proxy can handle gRPC traffic including streaming

3 participants