Kestrelhttpserver: Connection header "close" hangs

[jdslavin]

Request hangs when doing an HTTP GET request that has a "Connection" header of "close".

If the request is made with "Connection" header "keep-alive" all works well.

This issue came up because we are using an apache HTTP client library that automatically adds the Connection Header "close" when making non connection pool based requests.

[benaadams]

Dupe https://github.com/aspnet/KestrelHttpServer/issues/1041 ? Looks fixed in 1.1.0

[jdslavin]

I am not sure this is a duplicate, the close is sent on the request header where 1041 possibly looks like it deals with the response headers. Easy enough to test though, any GET request with Connection "close" in the request headers hangs.

[cesarblum]

@jdslavin What version of Kestrel are you using?

[jdslavin]

1.0.0 - I just finished upgrading from RC1 to 1.0.0 and this problem showed up.

[halter73]

@benaadams I agree that this looks like it could be a dupe of #1041, but even before the fix Kestrel would always close the connection given a "Connection: close" request header which you would think would prevent most clients from hanging. The issue was not setting the right/best response headers.

@jdslavin To be sure, can you provide us with a sample app? If this happens with any app or with our static file handler, you can just say so.

As far as the client goes, are you using the Apache's Java HttpClient? Can you reproduce the issue with Fiddler/Postman or something like that?

If you want to see if this has been fixed yourself, you can try using the 1.1.0-* preview bits from our nightly myget feed

[halter73]

One issue I've seen with "Connection: close" requests is that some clients won't send a FIN until a response has been received. This makes it impossible to ever complete a call to Request.Body.ReadAsync(...) for such a request.

In #406, which was fixed in rc2, we stopped trying to consume the entire body in Kestrel itself for these types of request, but it could be that the app is doing it.

[jdslavin]

I reproduced this same issue via Advanced Rest Client by just adding the "Connection: close" request header. I will try the preview bits to see what happens.

[halter73]

@jodavis Is the app reading the request body? Do you have an app it repros with? Or does it repro with the static file middleware (with no other middleware set up)?

[jdslavin]

I narrowed this down much further, I tried this under 1.1.0-alpha1-22131 with the same results BUT it turns out this is not a general problem because I can get static files and hit MVC endpoints just fine with that header. Where I run into the problem is I am hosting an OWIN based server (IdentityServer3) using the OWIN pipeline. I am trying to hit an endpoint based within the IdentityServer3 dll and that is where my error occurs.

In this case I'm hitting the openid connect well known endpoint and see this behavior: http://localhost:5001/core/.well-known/openid-configuration

To insert the IdentityServer3 I wrote a small extension to insert it via the aspnet owin component. See attached screenshot.

I am not sure at this point if its Kestrel, OWIN or the IdentityServer3 component that is causing this behavior. I have worked around the issue personally so its not blocking me and we can close the issue with Kestrel.

[halter73]

Can you try using IdentityServer4?

IdentityServer3 is likely trying to read from the request body and hanging since the client is sending the Connection: close header and the client is closing the connection until a response is received. Unfortunately the response won't be sent until the body is read, leading to a deadlock of sorts between the client and the server. Each waiting on the other.

After testing the behavior of IHttpHandlers and IIS, I am now convinced this is still a Kestrel bug. Request.GetBufferlessInputStream().Read(...) will return 0 for Connection: close GET requests without a content-length or chunked encoding. A POST request with the same headers will result in the handler not being run and IIS automatically returning a 411 Length Required HTTP status.

I think Kestrel should return a 411 in this case for POST/PUT/PATH/(anything else). And for all other HTTP methods (e.g. GET/HEAD/OPTIONS/...), Kestrel should assume the content length is zero if no Content-Length or Transfer-Encoding headers. The latter is already done for keep-alive connections.

@benaadams @cesarbs Do either you have a good list of HTTP methods that should require a content length?

[halter73]

We'll also have to be careful not to mess up Connection: Upgrade requests when making this fix.

[jdslavin]

I will give a shot at trying a simple IdentityServer4 tomorrow and see how that goes. I will keep you posted.

[benaadams]

I think its more nuanced http://tools.ietf.org/html/rfc7230#section-3.3.2

A user agent SHOULD send a Content-Length in a request message when no Transfer-Encoding is sent and the request method defines a meaning for an enclosed payload body.

So that isn't a rejection; however http://tools.ietf.org/html/rfc7230#section-3.3.2

A server MAY reject a request that contains a message body but not a Content-Length by responding with 411 (Length Required).

So I'd interpret that as assume content-length 0; when none - else there is _any_ body data; in which case reject? But that might be tricky to implement?

Otherwise

• No-body requests: GET, HEAD, DELETE, TRACE
• Optional body request: OPTIONS (Naturally :stuck_out_tongue: )
• Body requests: POST, PUT, CONNECT, PATCH

[benaadams]

Only issue with the assume 0 when it has data is if the body cannot be detected upfront, it will 400 the next request.

However I don't know if the body being empty can be checked without confirming the following data is a new request?

[benaadams]

Bleh

A user agent SHOULD NOT send a Content-Length header field when the request message does not contain a payload body and the method semantics do not anticipate such a body.

There is a discussion over here that is relevant Apache Traffic Server

Might even be something to invoke @blowdart on :unamused: as it is interpretive and has choices...

[jdslavin]

I tried this with the IdentityServer4 sample project and it works fine with the Connection "close" header. https://github.com/IdentityServer/IdentityServer4.Samples

[blowdart]

Should != MUST. So Kestrel ought to work both when the request does the right thing and when it doesn't.

[cesarblum]

My take on this:

• Enforce Transfer-Encoding/Content-Length on body requests. Reject with 411 if absent.
• Assume 0 for non-body requests. It's fine to 400 the next request since this is a client error (it shouldn't have sent a body, so that's bogus data in the pipes).
• Enforcing Transfer-Encoding/Content-Length where expected should be configurable but enabled by default. If an app chooses to disable it, it's up to it to determine what to do with such requests.

[halter73]

Right now for keep-alive requests, we always assume the content length is zero if the is no Content-Length or Transfer-Encoding header. First, we should change Kestrel to have the same behavior for Connection: close requests. For non-keep-alive requests, it's even safer since we won't try to process subsequent request over the same connection.

If we decide to require content length or chunking for POST, PUT, CONNECT, PATCH, we could do that for both keep-alive and non-keep-alive requests in a separate PR.

[cesarblum]

Sounds good. I can take care of this.