aspnet / KestrelHttpServer

[Archived] A cross platform web server for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
2.63k stars 528 forks source link

Add HTTP/2 request body data rate limit #3051

Closed halter73 closed 5 years ago

halter73 commented 5 years ago

Addresses #2808

Tratcher commented 5 years ago

Can we get a high level summary? This is pretty complex.

halter73 commented 5 years ago

@Tratcher Sure.

This moves the request body read timing logic from Http1MessageBody.PumpAsync to the shared HttpMessageBody.ReadAsync/CopyToAsync.

The old PumpAsync code had inverted logic where it paused the timer when waiting on the application in PipeWriter.FlushAsync. The new logic runs the timer when waiting on data from the other side in PipeReader.ReadAsync.

The logic, while inverted, is functionally the same. In both cases, any tick where any time is spent awaiting a read, is counted as a full tick awaiting a read due to the broad granularity of the timer.

I renamed some of the ITimeoutControl methods to reflect this inverted logic and the fact that there now needs to be an explicit call after starting the request body reads to time said reads.

The TimeoutControl implementation needed some adaptation to support HTTP/2. This includes:

  1. Supporting multiple concurrent timed reads that only need meat the min rate limit in aggregate.
  2. Disabling the timer when the connection-level HTTP/2 input flow control window is small enough that we can deduce it's the server, not the client, that's responsible for limiting the request data rate.
  3. Like before, the rate limit is enforced cumulatively for the entire request body. If there are multiple request bodies being read at once, this cumulative tracking is only reset once there a no request bodies being read at a point in time.

Since the rate limit is enforced in aggregate across all the streams in HTTP/2, any modifications to the per-request rate limit via the HttpContext are ignored for HTTP/2 requests.

Tratcher commented 5 years ago

Should we take the IHttpMinRequestBodyDataRateFeature and IHttpMinResponseDataRateFeature off HttpProtocol and only implement them for HTTP/1?

halter73 commented 5 years ago

Should we take the IHttpMinRequestBodyDataRateFeature and IHttpMinResponseDataRateFeature off HttpProtocol and only implement them for HTTP/1?

Seems reasonable. We already do this for IHttpUpgradeFeature. Technically it's a breaking change, even if it's not at the API level. At least the features are already absent on HttpSysServer, so we wouldn't break any apps that work cross-server.