Skip to content

GET to a URL with a redirection never completes nor times out #574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
0xpablo opened this issue Apr 3, 2022 · 2 comments · Fixed by #580
Closed

GET to a URL with a redirection never completes nor times out #574

0xpablo opened this issue Apr 3, 2022 · 2 comments · Fixed by #580
Labels
kind/bug Feature doesn't work as expected.

Comments

@0xpablo
Copy link
Contributor

0xpablo commented Apr 3, 2022

Hi there,
I realized some requests in our backend are getting stuck, they don't complete or finish unless I set a deadline.
Upon further investigation, the requests that are getting stuck are ones with a redirect. Doing a HEAD instead of a GET does result in a response (with a 301 status when disabling redirects in the client).

I'm trying to use the FileDownloadDelegate and I also noticed that nothing is getting called (no calls to reportHead or reportProgress). But as I cannot issue a simple GET I guess that the issue is not in the FileDownloadDelegate.

This is some sample code that reproduces the issue:

import AsyncHTTPClient

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)

let url = "https://ipfs.infura.io/ipfs/QmVES23Brg2HP2Cn5AEzkiToYpAPxvYfXrHbrg91ScLUES/image.jpeg"

let response = try httpClient.execute(request: .init(url: url, method: .GET)).wait()

print(response)

When running that on my Mac (Xcode 13.3 Apple Swift version 5.6 (swiftlang-5.6.0.323.62 clang-1316.0.20.8)), the request never completes.

Changing the .GET to a .HEAD does make the request complete successfully.


To workaround this issue, I'm creating an HTTPClient that does not follow redirects, to be able to inspect the location header and then use that URL on the client that downloads files.
I also noticed that if you do a HEAD request to a URL that will perform a redirect, there seems to be no way of obtaining the redirected URL (hence having to disable redirects on the workaround client). That would also be a nice to have.

Might be related to #538

@0xpablo
Copy link
Contributor Author

0xpablo commented Apr 4, 2022

I was able to investigate a bit more. It looks like the RequestBag receives the response head (receiveResponseHead0), which is processed by the state machine and leaves the state as .redirected(head, redirectURL).

Then the RequestBag receives response body parts (receiveResponseBodyParts0).
The state machine does not return a forward buffer (I understand that it's because we don't care about the body since it's a redirect), the RequestBag does not forward that to the delegate or consumes more body data, and this is where the request gets stuck.

One solution could be to succeed the request in receiveResponseHead0, which would cause the redirect to be performed, but two things come to mind:

  • The original request is not cancelled at that point, so we will still receive body parts, which triggers the preconditionFailure of RequestBag.StateMachine.receiveResponseBodyParts case .finished(error: .none).
  • I'm not sure if cancelling the original request is always a good idea, since the redirect could be in the same host and the connection could be reused. I'm not too familiar with Swift-NIO or async-http-client so I'm not sure if this would be handled by a lower-level layer and we don't need to worry about it.

I understand that making AHC a 3-tier library would probably make it easier to implement redirects more reliably, but it would be great to fix this for AHC v1.

@Lukasa
Copy link
Collaborator

Lukasa commented Apr 4, 2022

Yeah, for a redirect we probably need to consume the body entirely.

@Lukasa Lukasa added the kind/bug Feature doesn't work as expected. label Apr 4, 2022
fabianfett added a commit to fabianfett/async-http-client that referenced this issue Apr 11, 2022
### Motivation

Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.

### Changes

- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb

### Result

Redirect responses are consumed. Fixes swift-server#574
fabianfett added a commit to fabianfett/async-http-client that referenced this issue Apr 11, 2022
### Motivation

Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.

### Changes

- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb

### Result

Redirect responses are consumed. Fixes swift-server#574
fabianfett added a commit to fabianfett/async-http-client that referenced this issue Apr 12, 2022
### Motivation

Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.

### Changes

- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb

### Result

Redirect responses are consumed. Fixes swift-server#574
fabianfett added a commit to fabianfett/async-http-client that referenced this issue Apr 12, 2022
### Motivation

Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.

### Changes

- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb

### Result

Redirect responses are consumed. Fixes swift-server#574
fabianfett added a commit that referenced this issue Apr 12, 2022
### Motivation

Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.

### Changes

- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb

### Result

Redirect responses are consumed. Fixes #574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants