Skip to content
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

[ConnectionPool] Preserve connection errors #421

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

fabianfett
Copy link
Member

Motivation

If we can't establish a connection to a remote for the user, we will retry to create the connection. Eventually the requests connection timeout will fire, and we will fail the request. Right now we fail the request always with the HTTPClientError.getConnectionFromPoolTimeout error. While this is very descriptive, it does not tell the user if there are to many waiting requests or if there was a connection problem.

Changes

  • If errors occur during connection creation, we will save the error in the state machine
  • If a request times out, after we have seen a connection creation error, we will fail the request with the connection creation error.

Result

If user have a typo in their url, they will get better errors. (dns failure)

@fabianfett fabianfett changed the title Preserve connection errors for user [ConnectionPool] Preserve connection errors Sep 10, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 10, 2021
@fabianfett fabianfett added semver/none For PRs that when merged do not need a bump in version number. semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) and removed semver/none For PRs that when merged do not need a bump in version number. labels Sep 10, 2021
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM.

@@ -26,6 +26,8 @@ extension HTTPConnectionPool {

private var connections: HTTP1Connections
private var failedConsecutiveConnectionAttempts: Int = 0
/// the error from the last connection creation
private var lastConnectFailure: Error?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible future enhancement: store a vector of errors and throw a single, composite error that encompasses them all.

@fabianfett fabianfett merged commit 0c36de2 into swift-server:main Sep 13, 2021
@fabianfett fabianfett deleted the ff-better-error-messages branch September 13, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants