Always start the response before draining the request.

[Tratcher]

#2102 This design issue was negating the purpose of 100-Continue by sending it even when the app didn't read the body such as 401s, 404s, 301s, etc., and it added latency to responses without bodies. It was designed that way to allow reporting request body errors to the client. However, most request body errors are caused by client disconnects or timeouts and require the connection to be closed anyways. Regardless the app's response should be given priority over body drain errors.

Only six tests were affected by the change. Two were testing this specific functionality. The other four were only using this as a means to report test results. We may need to re-work some of these tests to verify the results another way.

New tests in #2106 may also need to be updated.

[halter73]

I don't think this PR is worth the risk for the reasons I explained in #2102 (comment).

[Tratcher]

One buggy client (that we control) does not warrant breaking 100-Continue functionality for all other clients. Other servers do not do this.

[Tratcher]

@shirhatti

[Tratcher]

@halter73 you yourself pointed out that your risk assessment is at odds with the proposed workaround of manually flushing the response. Manually flushing carries the same risk because it also causes the 100 continue to not be sent and triggers the same bug in the client.

Similarly there is no proper server-side workaround for the client bug for responses that have bodies (when you are rejecting the request without consuming it). The proper workaround for the client bug is for the client to disable Expect: 100-Continue and always send the request body.

[halter73]

The manual flush workaround to achieve the would-be-automatic-behavior-of-this-pr would carry the same risk as the automatic behavior of this PR. I don't recommend this workaround any more than I recommend merging this PR because for this reason. Still, it's worth noting that an app code change can mimic behavior of this PR on a more targeted basis.

To workaround the Linux HttpClient bug, you need to either not manually flush, or you need manually drain the request body before flushing. Obviously, it's more waay more common for apps to do the former and not flush. These apps would be the ones broken by this PR.

Also, how sure are you that other servers don't send 100-continue responses in this situation? I know a lot of servers buffer request bodies before calling app code. It's also telling that the first report of the Linux HttpClient bug was found making requests to Kestrel. Especially considering you need quite the unusual app to even induce the Linux HttpClient bug using Kestrel today, it's surprising the bug wasn't found with other servers if they already behave in a way that induces the bug.

[Tratcher]

To clarify, there are two classes of workarounds. 1) Apps trying to work around Kestrel's bug so they can send final status codes without 100's to working clients, and 2) Apps trying to work around HttpClient's bug to avoid protocol corruption (and thus crippling 100s for all clients).

Reviewing this, the strangest part of the whole thing is that kestrel is treating responses with and without bodies differently. This leaves the app half broken for both working and broken clients. Working clients don't get the final status code they need for responses without bodies, and broken clients don't get the 100 status code they need for responses with bodies. Regardless of trying to mitigate the client bug, Kestrel should at least be consistent. It should either always or never send the 100 ahead of final responses that have not read the body. Whether a response has a body or not should not have any bearing on how the request gets treated.

[halter73]

It would be safer to always send a 100 ahead of final responses for relevant requests. The downside is that this would remove the ability to manually get the automatic behavior of this PR, but I don't think that's too important. I would do this or keep the current behavior.

[Tratcher]

Corefx is addressing their end by disabling ExpectContine by default for CurlHandler and by either sending the content or closing the connection for ManagedHandler.
dotnet/corefx#24789

In the process we found that CurlHandler/libcurl isn't as broken as originally thought. It only has the problematic behavior for 2xx responses. For 300+ it uses a number of metrics to decide if it should send the body or close the connection. This covers the common auth and redirect scenarios, and ignoring the request body for a 2xx is far less common. Any affected clients can opt-out of 100-continues, or the app/server could mark those 2xx's as Connection: close.

We should be able to proceed with this PR now. I'll check the HTTP/2 100-continue behavior that was recently added and update the PR as needed.

[Tratcher]

Rebased. The new HTTP/2 tests from #2106 weren't affected, they're much less extensive.

That leaves the kestrel tests that were using this mechanic to flow request body drain errors. I adjusted them to work with this change but they should be re-written to more directly test the features they were targeting. @halter73 I'll sync up with you next week for suggestions on those.

[Tratcher]

Rebased with test comment updates. The tests are all accurate but we've lost some visibility for request format errors and timeouts during drain, they primarily surface as disconnects now. That said, request drain errors and timeouts are among the least important type of failure, the error gets logged and the connection dropped but otherwise the the application does not care.

Ready for final review, unless you have suggestions for surfacing these exceptions from the tests in another way. E.g. an internal event for drain errors.

[Tratcher]

Tests updated.

[halter73]

Do expectedClientStatusCode and expectedServerStatusCode always match now?

[Tratcher]

No. Some tests set null vs 0, and one sets null vs 400.