swift-server / async-http-client

HTTP client library built on SwiftNIO
https://swiftpackageindex.com/swift-server/async-http-client/main/documentation/asynchttpclient
Apache License 2.0
912 stars 117 forks source link

`Connection refused` not throwing right away #743

Closed andreasley closed 3 months ago

andreasley commented 3 months ago

AsyncHTTPClient commit hash: a22083713ee90808d527d0baa290c2fb13ca3096 (Release 1.21.1)

While trying to use HTTPClientRequest on macOS (with NIOTransportServices), I noticed that refused connection errors are only thrown when the connection timeout expires. With the following code, it takes 90 seconds (the connection timeout specified in HTTPClient.Configuration.singletonConfiguration) until the error is thrown:

import AsyncHTTPClient

var request = HTTPClientRequest(url: "https://localhost:12345/")
let response = try await HTTPClient.shared.execute(request, timeout: .hours(1))

I'd consider a refused connection a non-recoverable error and would expect execute to throw right away.


Environment: $ swift --version swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4) Target: arm64-apple-macosx14.0

Operating system: macOS Sonoma 14.5 (23F79)

andreasley commented 3 months ago

The property networkFrameworkWaitForConnectivity in a HTTPClient.Configuration looked promising, so I've tried setting that to false, but that just results in continuous retries every ~100ms until the timeout expires.

Lukasa commented 3 months ago

Connection refused is not considered non-recoverable. There are plenty of circumstances in which a reconnect attempt may succeed, including VPN changes, network state changes, and getting different DNS results. Generally speaking we attempt to retry. I'm a bit surprised that you're seeing a retry every 100ms, as the default connect timeout is 10s. You can override those settings in the Configuration struct as well, using Configuration.timeout.

andreasley commented 3 months ago

While I agree that changes in the network configuration may allow a reconnect attempt to succeed eventually, there are situations in which a user expects to be informed of a failed connection attempt right away. This would also match the behavior of Swift's URLSession and most applications (e.g. Safari) which instantly fail on a refused connection.

Documentation in NIOTransportServices.NIOTSNetworkEvents.WaitingForConnectivity states the following regarding these transientErrors:

/// Note that these reasons are _not fatal_: applications are strongly advised not to treat them
/// as fatal, and instead to use them as information to inform UI decisions.

Unfortunately, I did not find any way to surface these errors to the UI when using HTTPClientRequest.

A very low timeout would shorten the user's wait time but risks cutting off legitimate connection attempts, so that's not a great solution.

If HTTPClient.Configuration.ConnectionPool's retryConnectionEstablishment would be public, consumers if the library could use that together with networkFrameworkWaitForConnectivity = false to receive connection errors before the timeout elapses. Is that something you'd be willing to consider?

Lukasa commented 3 months ago

I think a reasonable feature enhancement would be to have some filtering logic that assumes that some errors are going to be fatal, and to allow users to configure a fast-fail in that case.

andreasley commented 3 months ago

I may have edited my previous post at the worst possible time and I'm not sure if you've seen the suggestion about making retryConnectionEstablishment public, which would allow to optionally fast-fail with minimal changes to the code.

Lukasa commented 3 months ago

I think that was supposed to be public and simply got missed in code review, and the @testable import annotation hid it from us. I'd be happy to merge a PR that makes it public.