Fix redirects following POST/PUT/PATCH requests

[agisboye]

I’ve stumbled upon a problem in URLSession when a redirect occurs following a request with a request body (POST/PUT/PATCH).

Say we create a URLRequest with a httpBody and an httpMethod of "POST".

When the POST request is made, a URLSessionTask is initialized with the httpBody from the URLRequest.

Later, when the server responds with a redirect to this request, a new request is made but the original task is reused. This involves calling startNewTransfer, which in turn looks up any existing known body in the knownBody property.

The result is that the new request, which is supposed to follow the redirect, also includes the original request data. This trips the following check.

This PR attempts to work around the issue by unsetting the knownBody on URLSessionTask before a redirect request is performed.

I also changed the behavior of how the redirect request is constructed. Previously, the original request (including httpBody) was copied and the httpMethod changed to GET. This is an invalid request.
Even though this turned out not to be the source of the problem (because the httpBody on the redirect request is never used), it doesn’t make sense for Foundation to be producing invalid requests internally.

[spevans]

@swift-ci test

[spevans]

I ran the test case against macOS Foundation (using the DarwinCompatibilityTests project) and it timed-out. I don't know if this is a bug in macOS or an incorrect test case.

[agisboye]

It seems several of the tests for URLSession fail with a timeout (when testing against macOS Foundation). This happens on commits prior to mine as well so I don't think it's related to the changes that I've made. Could it be an issue with the test server implementation?

[spevans]

The reason some of the tests hang on macOS is that the delegates (when used) should be calling a completion handler with .allow to keep the transaction progressing. There is a bug in URLSession on Linux that doesn't require this completion handler call so the buggy tests still work on Linux. Other tests are failing for different reasons.

My current plan for URLSession is to re-enable the current tests on CI (except on Windows where there are still some issues) and then fix up each test to work on macOS, while fixing URLSession to correctly pass the fixed tests and fail with the broken tests. Once all of the tests are passing on macOS then the other bugs can be fixed up.

So I would suggest re-examining your test to see why it is failing on macOS as it may indicate another bug in the Linux code.

[agisboye]

It seems DataTask never calls the completion handler here.
I'd rather not change the behavior of DataTask since it's used by so many of the tests. Instead I'm using HTTPRedirectionDataTask.

[spevans]

@swift-ci test

[spevans]

@swift-ci test

[spevans]

This looks fine now, could you just squash the commits down to one please, and then this can be merged in. Thanks

[spevans]

@swift-ci test and merge