fix handling of client Close before writing entire Content-Length #20428
Conversation
|
@hughbe There's likely a similar issue with chunked encoding, if you're looking for something to test :) |
|
|
||
| public static void WaitForSocketShutdown(Socket socket) | ||
| { | ||
| if (PlatformDetection.IsWindows || PlatformDetection.IsOSX) |
There was a problem hiding this comment.
So I found that the tests intermittently failed on Windows if I didn't have this code. See https://github.com/dotnet/corefx/issues/18188
Is this something you found?
There was a problem hiding this comment.
I didn't see this, but now you have me nervous that I didn't test it properly on the Windows impl... running those tests again now.
There was a problem hiding this comment.
I just ran the tests several times and didn't see any failures.
| await Assert.ThrowsAsync<HttpListenerException>(() => context.Request.InputStream.ReadAsync(buffer, 0, buffer.Length)); | ||
|
|
||
| // Closing a response from a closed client if no writing has failed should fail. | ||
| Assert.Throws<HttpListenerException>(() => context.Response.Close()); |
There was a problem hiding this comment.
Why did you remove this test and the one below?
There was a problem hiding this comment.
Because it seemed like weird behavior. The client has sent a malformed request; once I get the exception on reading the request, what else am I supposed to do except Close? I would understand if Write threw, but Close seems weird.
If this is Windows behavior, then maybe we should consider whether that should change.
Thoughts?
There was a problem hiding this comment.
Because it seemed like weird behavior. The client has sent a malformed request; once I get the exception on reading the request, what else am I supposed to do except Close? I would understand if Write threw, but Close seems weird.
I'd argue the point of tests like this is to demonstrate the weird behaviour! I'm perhaps unreasonably against deleting tests ever without good reason.
If this is Windows behavior, then maybe we should consider whether that should change.
Unlikely that's going to happen - HttpListener in netcoreapp is a legacy API and breaking changes aren't likely to be accepted. From @karelz in #19437
I would suggest to close it, unless:
We see customers hitting it hard (which is unlikely as it was not reported even on .NET Framework AFAIK)
Fix is simple and someone submits it
Otherwise I would not invest in legacy APIs to this level ... thoughts?
There was a problem hiding this comment.
Yeah, good point, it's not going to change on Windows.
That said, I do think it's weird behavior, and it's not clear that matching the Windows behavior is worthwhile.
There was a problem hiding this comment.
The thing is that the Windows implementation isn't really the Windows implementation, it's the Http.sys implementation. It's my understanding that in the 2.1 timeframe UWP/UAP/whatever it is will use the managed implementation, so the compat burden in terms of managed vs Http.sys is higher. I don't feel strongly about this, but maybe let's keep the test and wrap it in a if (Helpers.IsWindowsImplementation...)
| if (_remainingBody >= 0 && size > _remainingBody) | ||
| { | ||
| size = (int)Math.Min(int.MaxValue, _remainingBody); | ||
| size = (int)Math.Min(_remainingBody, (long)size); |
There was a problem hiding this comment.
What happens if size end up being 0? Do we potentially hang in a 0-byte read if the connection is left open?
There was a problem hiding this comment.
Yeah, there's a missing check here. If _remainingBody is 0 we should return 0.
There was a problem hiding this comment.
Actually, this is handled. FillFromBuffer returns -1 if _remainingBody is 0.
|
@dotnet-bot test Innerloop netfx Windows_NT Debug x86 |
|
All of the NetFX failures are due to an EventListener bug on .NET 4.6.1. |
|
@dotnet-bot test outerloop netcoreapp Windows_NT Debug |
|
@dotnet-bot test Innerloop netfx Windows_NT Debug x86 Build and Test please (EventListener bug) |
|
@mmitch, in the OSX leg after all tests passed: |
6 participants
If the client closes the connection before writing the full body, as specified by the Content-Length header, we should throw an exception on InputStream.Read/ReadAsync.
Partial fix for #20246.
@stephentoub @hughbe