Add HttpListenerResponse tests #19880
Conversation
That's excellent. Thanks, @hughbe! |
| int bytesReceived = Client.Receive(buffer); | ||
| Assert.True(bytesReceived < buffer.Length, "Buffer should hold enough space to hold the response from the server."); | ||
|
|
||
| return Encoding.UTF8.GetString(buffer, 0, bytesReceived); |
There was a problem hiding this comment.
This method has inherent race conditions. Receive won't guarantee that it'll receive everything sent by the server. Assuming the server closes the connection, it really needs to repeatedly receive until it gets back 0, indicating that it won't get any more (or if the connection isn't getting closed, it needs to follow the HTTP spec for determining when content for the request is done, e.g. based on Content-Length or chunked transfers). I think you're better off going back to using HttpClient, and then using ReadAsStringAsync on the response.
There was a problem hiding this comment.
The reason why I went with Socket instead of HttpClient is because HttpClient doesn't seem to read some of the headers that are sent back from the HttpListener - this could be because they are invalid, for example. Socket allows me to actually verify we send what we expect. HttpClient simply sets headers it doesn't like to null, for example.
I thought that doing synchronous writes and reads would avoid this issue - I can look into using HttpClient again, but it'd be nice to have some kind of way to read the exact response from the server.
There was a problem hiding this comment.
I thought that doing synchronous writes and reads would avoid this issue
Doesn't matter whether they're synchronous or asynchronous. There's still no guarantee you'll read the entire response in one call. In practice, given the small size of these buffers, you're likely to, but it's a recipe for spurious failures happening here and there in CI.
it'd be nice to have some kind of way to read the exact response from the server.
You can use a socket, you'll just need to either:
a) ensure that the HttpListener is closing the connection such that the client can read to the end, and then you can either loop until Receive returns 0, or wrap the socket in a NetworkStream and Read from it until that returns 0 or use its CopyTo or wrap that in a StreamReader and use ReadToEnd
b) know exactly how many bytes you're sending back and read until you get that exact amount
c) implement a more HTTP-protocol aware mechanism, which you don't want to have to do
There was a problem hiding this comment.
@hughbe please try to reuse HttpsTestClient if custom HTTP client processing is required.
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(value)); | ||
| } |
There was a problem hiding this comment.
Why was this necessary? Doesn't both the desktop and the Windows implementation null ref in the same situation? At this point let's avoid making unnecessary behavioral changes like this. We should stick to only adding tests that pass on Windows and then fixing the managed implementation to pass. If we find bad bugs in the Windows implementation as well, we could look to fix those, but this wouldn't count as a bad bug.
| if (templateResponse == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(templateResponse)); | ||
| } |
|
Great set of tests, @hughbe. It looks good. Let's undo the product changes you made (unless I'm missing something) and fix up the get response method in the tests, and then I think it should be good to go. |
| Client.Send(Factory.GetContent(httpVersion, "POST", null, "Give me a context, please", null, headerOnly: false)); | ||
| HttpListenerContext context = await Factory.GetListener().GetContextAsync(); | ||
| return context.Response; | ||
| } |
There was a problem hiding this comment.
Wow! You wrote a lot of tests here!
:) Still this file is way too big. Please split into separate classes of tests.
There was a problem hiding this comment.
It's the sort of testing we need to be confident we're not breaking the managed implementation!
I thought the testing guidelines are file per class? I'm happy to break up the tests though - is this because you're concerned about test execution time and want the test classes to run in parallel?
There was a problem hiding this comment.
I thought the testing guidelines are file per class? I'm happy to break up the tests though - is this because you're concerned about test execution time and want the test classes to run in parallel?
Take a look at HttpClient tests.
https://github.com/dotnet/corefx/tree/master/src/System.Net.Http/tests/FunctionalTests
There are several files such as HttpClientHandlerTest.cs, HttpClientHandlerTest.DefaultProxyCredentials.cs etc.
There was a problem hiding this comment.
It's the sort of testing we need to be confident we're not breaking the managed implementation!
Yup: thanks a lot for writing these!
I'm happy to break up the tests though - is this because you're concerned about test execution time and want the test classes to run in parallel?
No, it's just that in general, it's bad practice to have a single huge class in a single huge file.
| public class HttpListenerResponseTests : IDisposable | ||
| { | ||
| private HttpListenerFactory Factory { get; } | ||
| private HttpClient Client { get; } |
There was a problem hiding this comment.
I actually like the fact we're using HttpClient: we should continue to do so to ensure that the client/server stacks can communicate with themselves. (It's less interesting that a custom test client can achieve the same thing.)
That said, you could duplicate all tests here into another file to the granular control through a Socket and a custom HTTP test server.
| // The managed implementation should set ContentType directly in the headers instead of tracking it with its own field. | ||
| // The managed implementation should trim the value. | ||
| [ActiveIssue(18128, TestPlatforms.AnyUnix)] | ||
| public async Task ContentType_SetAndSend_Success(string contentType) |
There was a problem hiding this comment.
@hughbe Would it be better to file separate issues for identified similar compat differences? In this case filing a separate bug saying that response headers require additional validation and stored in place. So that work can be done incrementally and also distributed in case you are otherwise occupied. Linking all to a single issue doesn't give a clear picture of how much work is still left before one can call the umbrella issue done.
|
By the way can we hold off merging this until I push another commit - I noticed that a test randomly failed so there is a race condition at play that Stephen mentioned. Examples: CloseResponseEntity_ChunkedSentHeaders_DoesNotModifyContentLength(willB lock: True)Assert.EndsWith() Failure: Expected: 5 Hello 1 a 0CopyFrom_AllValues_ReturnsCloneCloseResponseEntity_NotChunkedSentHeaders_SendsEntityWithoutModifyingContentLengthCloseResponseEntity_ChunkedNotSentHeaders_ModifiesContentLength |
There was a problem hiding this comment.
Left a few review comments that are now hidden by the recent changes.
In summary:
- I think we should reuse existing HTTP test client code instead of creating then stabilizing a new version.
- In general, if GitHub believes that a file is too large to render, the file should be split for easier readability, encapsulation, etc.
|
@CIPop I could not get HttpsTestClient to work for several reasons:
I went with stephen's second suggestion about hardcoding the byte length to avoid having to rearchitecture HttpsTestClient |
HttpsTestClient is currently supporting only HTTPS connections but it can be refactored to support non-HTTPS, multiple callbacks, etc. |
There was a problem hiding this comment.
Thanks @hughbe for your work and the extra tests!
Please ensure that all InnerLoop tests pass.
Also, use the @dotnet-bot help to find out how to run all the OuterLoop tests before we can merge this in order to avoid test failures in the daily runs (most of the network tests are disabled in Inner).
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Debug |
|
@dotnet-bot test outerloop netcoreapp Windows 7 Debug |
- Remove added argument validation - Split up tests - Start to investigate fixing race conditions in the tests (this isn't done yet)
Fixes failures when I stress test the suit by running the tests 100 times
…instead of ActiveIssue This lets me simulate being on a managed environment on Windows so I can iterate more rapidly
| { | ||
| // Most of the time, the Socket can read the content send after calling Close(byte[], bool), but | ||
| // occassionally this test fails as the HttpListenerResponse closes before the Socket can receive all | ||
| // content. If this happens, just ignore the failure and carry on. |
There was a problem hiding this comment.
This is in both the Windows and managed implementation, or just the managed one? This sounds like a bug. It should be possible for the server to close the connection and still have the client receive all of the data sent before that, assuming the server is doing a proper shutdown of the connection.
There was a problem hiding this comment.
I ran the tests several hundred times on Windows - this failed intermittently. Haven't had the chace to test with the managed implementation
There was a problem hiding this comment.
Thanks for your good work on this, @hughbe. Let's get this merged, and then we can follow-up to address any subsequent issues.
|
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Debug |
|
@hughbe, it looks like the outerloop Windows runs are hanging in the HttpListener tests. |
|
(though it looks like it may be one of the already checked in tests rather than one you've added here) |
|
Oof. Is it only Windows 7? How can you tell which test is hanging by the way? |
Looks like it's both 7 and 8. Can't tell which is hanging from the logs, but I see the master runs hanging as well, which would suggest there's a checked in test that's hanging. |
|
(I'm setting up a Win7 VM to see if I can figure out what's going wrong.) |
|
@dotnet-bot test outerloop netcoreapp Windows_NT Debug |
|
@mmitche, @danmosemsft, the Nano job failed with: Is that expected? Is this the wrong job to run for Nano? |
|
@stephentoub Run the new pipelines: @dotnet-bot test portable windows debug pipeline |
|
@mmitche, does that mean the run wasn't supposed to work? If so, can you please update the help message from dotnet-bot that explicitly lists it as valid? |
|
@stephentoub I thought it was removed from the file altogether. I'll fix that. |
|
Thanks. And these portable Windows runs are on Nano? |
|
@stephentoub They run on all the targeted windows portable platforms (nano, 10, 7, etc.). As of right now since the submission is part of the build, you can't select just one target platform to test on. |
|
@mmitche, ok. And it's outerloop, right? |
|
@stephentoub No. It's coded today but there isn't a trigger for it. Want me to add that? |
Yes, please. |
|
I'm going to merge this; after #20104 was merged, the legs that previously hung now pass. |
* Add HttpListenerResponse tests * Baseline all the managed test failures * Preliminary PR feedback - Remove added argument validation - Split up tests - Start to investigate fixing race conditions in the tests (this isn't done yet) * Baseline all HttpListener disabled tests on Unix * Use exact number of bytes to avoid race conditions Fixes failures when I stress test the suit by running the tests 100 times * Fix interminate test failures discovered by running the tests 100 times * Fix managed test failures and use overridable ConditionalFact/Theory instead of ActiveIssue This lets me simulate being on a managed environment on Windows so I can iterate more rapidly * Fix Windows 7 outerloop tests
9 participants
Contributes to #13618
I've baselined all the managed failures instead of trying to fix them in this PR - there are just too many!
I'm sure this will give me plenty of work to do after this PR. Considering that there's been some discussion of the risk of my fixes recently, I'm going to invest in more comprehensive testing, followed by targetted fixes
@stephentoub @Priya91 @davidsh