Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[release/2.1] Add support and tests for HTTP 308 Permanent Redirect (#30398)#30954

Merged
rmkerr merged 1 commit intodotnet:release/2.1from
rmkerr:308_servicing_port
Jul 10, 2018
Merged

[release/2.1] Add support and tests for HTTP 308 Permanent Redirect (#30398)#30954
rmkerr merged 1 commit intodotnet:release/2.1from
rmkerr:308_servicing_port

Conversation

@rmkerr
Copy link
Contributor

@rmkerr rmkerr commented Jul 10, 2018

The HttpStatusCode enum was only recently updated to include HTTP status code 308, via PR #26727. With the platform handlers we support whatever cURL and WinHttp support, so 308 works even though the status code did not exist in the enum until recently. However in SocketsHttpHandler we only support the redirects that were in the enum when that code was written, which at the time did not include 308.

This PR adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests.

This is an exact port of PR #30398.

Fixes: #30389

This change adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests.

Fixes: #30389
@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

@davidsh davidsh added this to the 2.1.x milestone Jul 10, 2018
@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

The OSX failures are unrelated. The other two are a bit more suspicious as they are in System.Net.Http, even though none of the failed tests are using redirects. I'm going to do a rerun of those to see if we see the same results.

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build

@danmoseley
Copy link
Member

Approved to merge pending tests/review signoff.

@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

There are three failures happening in the System.Net.Http tests. One is clearly unrelated, but unless I can find some evidence that the other two were already occurring I think we should hold off on merging this for 2.1.3. Details of the failures are below:

GetAsyncWithMaxConnections_DisposeBeforeReadingToEnd_DrainsRequestsAndReusesConnection:
This one doesn't touch redirects, but seems to be a new failure as it does not appear to have occurred recently in the daily runs on the 2.1 branch.

AuthProxy__ValidCreds_ProxySendsRequestToServer:
Similar to the issue above. There are no recent failures in this test, but it also does not use redirects.

Brotli_External_DecompressesResponse_Success:
This one has been occurring in the 2.1 release branch since before the PR. This is almost certainly unrelated.

@davidsh
Copy link
Contributor

davidsh commented Jul 10, 2018

GetAsyncWithMaxConnections_DisposeBeforeReadingToEnd_DrainsRequestsAndReusesConnection:
This one doesn't touch redirects, but seems to be a new failure as it does not appear to have occurred recently in the daily runs on the 2.1 branch.

Is this test failing on .NET Core or .NET Framework? This test is not reliable on .NET Framework and really only should be testing with .NET Core using SocketsHttpHandler. The test shouldn't run on any other handler as the "drain" behavior doesn't match SocketsHttpHandler.

@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

That failure is on .NET Framework. Thanks for the info -- I'll dig into the other failure and see if it is something similar.

@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

A baseline run of the 2.1 servicing branch with no changes did not show the same failure. Giving it one more try...

@dotnet-bot test Outerloop Windows x64 Debug Build

@rmkerr
Copy link
Contributor Author

rmkerr commented Jul 10, 2018

The tests aren't finished, but the test in question passed. I would prefer to have CI green, but there appear to be test failures on the 2.1 release branch without these changes. I'm confident that these changes are not related to the failures we saw, and am going to merge them.

@rmkerr rmkerr merged commit 35a009a into dotnet:release/2.1 Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants