Tornhoof / HttpBatchHandler

HttpBatchHandler for ASP .NET Core 3.0
Apache License 2.0
22 stars 7 forks source link

Does not work with proxy due to missing Content-Length #27

Open mcm-ham opened 3 years ago

mcm-ham commented 3 years ago

I'm finding requests inside batch are missing request body once forwarded by reverse proxies such as ProxyKit or YARP are missing. Looking into the source code I discovered this was due to missing ContentLength: https://github.com/ProxyKit/ProxyKit/blob/master/src/ProxyKit/HttpContextExtensions.cs#L59

After I manually added ContentLength header it now correctly works. However, I didn't see this being needed in the examples provided here: https://devblogs.microsoft.com/aspnet/introducing-batch-support-in-web-api-and-web-api-odata/

Tornhoof commented 3 years ago

Hello, thank you for your interest in httpbatchhandler.

The middleware is not setting the content-length header, this is done by the actual server implementation (above the middleware). In the tests the content-length is correctly set in the response. As for the test website, kestrel correctly uses Transfer-Encoding: chunked, which is the expected behaviour if Kestrel does not the length of the response (which it doesn't without buffering).

example:

HTTP/1.1 200 OK
Date: Wed, 20 Jan 2021 07:10:09 GMT
Content-Type: multipart/batch; boundary="8101b18f-02c6-42a2-92bb-e64bec026c8c"
Server: Kestrel
Transfer-Encoding: chunked

Your problem appears to be somewhere else, for example something stripping the transfer-encoding or changing the behavior, do you run yarp/proxykit in the same process as the middleware? If yes, I wouldn't be suprised if you run into problems, as they all change request handling quite a bit.

mcm-ham commented 3 years ago

thank you for your interest in httpbatchhandler.

Thanks for your work on HttpBatchHandler, we're finding it useful.

In the tests the content-length is correctly set in the response.

I'm referring to the individual requests after batch middleware i.e. if I modify the test website to add this:

app.Use(async (context, next) => {
    _logger.LogWarning($"Before Content-Length: {context.Request.Headers[HeaderNames.ContentLength]}");
    await next();
});
app.UseBatchMiddleware();
app.Use(async (context, next) => {
    _logger.LogWarning($"After Content-Length: {context.Request.Headers[HeaderNames.ContentLength]}");
    await next();
});

And then make a batch request like this:

var req = new HttpMessageContent(
    new HttpRequestMessage(HttpMethod.Post, "http://localhost:5123/api/values") { Content = new StringContent("5") }
);
await new HttpClient().SendAsync(new HttpRequestMessage(HttpMethod.Post, "http://localhost:5123/api/batch")
{
    Content = new MultipartContent("mixed", "batch_" + Guid.NewGuid().ToString()) { req, req }
});

You can see from the log output that the batch request has content-length but once split into 2 individual requests they no longer have content-length header.

warn: HttpBatchHandler.Website.Startup[0]
      Before Content-Length: 432
warn: HttpBatchHandler.Website.Startup[0]
      After Content-Length:
warn: HttpBatchHandler.Website.Startup[0]
      After Content-Length:
Tornhoof commented 3 years ago

Ah okay, now I understand, you mean Content-Length of individual requests after the batch fan out. HttpBatchHandler does not change the headers for those requests in any way, so if your original batched request contains no content-length then HttpBatchHandler does not add them. So yeah, your solution to add them individually should work fine.