Allow immediate request failure on connection error #625

dnadoba · 2022-09-06T14:44:11Z

Motivation

In integration tests we often want to inspect the connection error thrown during connection establishment for a request. Currently the only way to get at the connection error is to wait until the request timeout is expired. This is because of the connection retry behaviour backed into AHC which cannot be turned off. AHC will try to create new connections on connection failure with an exponential backoff. It stores the last connection and will fail the request if the request times out.

Modification

Adds a new internal flag called retryConnectionEstablishment to HTTPClient.Configuration.ConnectionPool.
If false, requests will fail immediately after a connection could not be established. Otherwise the old behavior is used which is still the default.

Set retryConnectionEstablishment to false to speedup test from ~40s to ~1s in HTTPClientNIOTSTests.swift. More tests will follow.

Result

Allows fast and reliable test execution.

Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift

`AsyncAwaitEndToEndTests.testImmediateDeadline` failed in CI. Hard to debug without more information. Doesn’t fail locally with 1000 repetitions.

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

fabianfett · 2022-10-06T10:59:51Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            guard self.retryConnectionEstablishment else {
+                return .init(
+                    request: self.failAllRequests(reason: error),
+                    connection: .none
+                )
+            }
+


This is weird! If we always fail all requests, when we a connection attempt failed, we should never reschedule a connection attempt. And this means we should never reach this point. Since we never schedule a backoff timer.

waitingForConnectivity is called if a NWConnection transitions into the waiting state which has nothing to do with our own connection retry logic. This means that it will be called if a NWConnection "fails" instead of failedToCreateNewConnection because the connection will do its own retry thing.

Can we please add a code comment, that this is only triggered in the Network.framework flow?

If we don't want to retry connection establishment, NW.framework should not go into the waiting state, should it?

I have removed the code again. We have a different flag (networkFrameworkWaitForConnectivity) for disabling Network.frameworks waiting behaviour.

fabianfett

And a bit more...

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift

fabianfett · 2022-10-06T11:03:34Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

@@ -219,6 +223,17 @@ extension HTTPConnectionPool {

            switch self.lifecycleState {
            case .running:
+                guard self.retryConnectionEstablishment else {


Instead of using the guard here it might be nicer to go with where clause in the switch...

A where constraint only applies to the last case if multiple cases are defined e.g.

case .running, .shuttingDown where self.retryConnectionEstablishment == true:

the where constrain only applies to .shuttingDown but not .running. We had an issue in AHC which we debugged together and decided we should stop using where clauses in switch cases altogether because it is not obvious.

true, however here we only have one case here. Anyway, not a hill I want to die on. If we want to stick with an inline if/guard, I would prefer this to be an if self.retryConnectionEstablishment {} and then move the retry logic into the if.

What's the benefit of the if? Guard at least guarantees that we return, if can fall through which we don't want.

I consider it nicer to read. But that's personal...

fabianfett · 2022-10-06T11:07:03Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift

@@ -401,13 +405,38 @@ extension HTTPConnectionPool {
            self.failedConsecutiveConnectionAttempts += 1
            self.lastConnectFailure = error

+            guard self.retryConnectionEstablishment else {


Why don't we check the current running state here? Like in the http1 case? This seems weird. I think we should add this here. If the pool is closing, we should not backoff anymore.

Yeah agree this is weird but has nothing to do with this change as we don't check the state at all. We should tackle this in a separate PR as we likely want to add test.

Sources/AsyncHTTPClient/HTTPClient.swift

fabianfett

We need TODOs for the incorrect http2 shutdown behavior, and we should change handling in waiting for NW.framework and/or document it better.

fabianfett · 2022-10-10T10:00:28Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

@@ -219,6 +223,17 @@ extension HTTPConnectionPool {

            switch self.lifecycleState {
            case .running:
+                guard self.retryConnectionEstablishment else {


true, however here we only have one case here. Anyway, not a hill I want to die on. If we want to stick with an inline if/guard, I would prefer this to be an if self.retryConnectionEstablishment {} and then move the retry logic into the if.

fabianfett · 2022-10-10T10:02:51Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            guard self.retryConnectionEstablishment else {
+                return .init(
+                    request: self.failAllRequests(reason: error),
+                    connection: .none
+                )
+            }
+


Can we please add a code comment, that this is only triggered in the Network.framework flow?

If we don't want to retry connection establishment, NW.framework should not go into the waiting state, should it?

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift

fabianfett

Thanks for pushing through!

Allow immediate request failure on connection error

ab6142b

dnadoba added the 🔨 semver/patch No public API change. label Sep 6, 2022

generate linux tests and run SwiftFormat

cfce59e

Lukasa requested a review from fabianfett September 6, 2022 16:45

Merge branch 'main' into dn-allow-disabling-connection-retry

de02ba3

fabianfett reviewed Sep 20, 2022

View reviewed changes

Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift Outdated Show resolved Hide resolved

dnadoba added 2 commits September 20, 2022 15:26

remove copy & past comments

2cff156

print unepxected error

8a1f97d

`AsyncAwaitEndToEndTests.testImmediateDeadline` failed in CI. Hard to debug without more information. Doesn’t fail locally with 1000 repetitions.

fabianfett reviewed Oct 6, 2022

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift Show resolved Hide resolved

fabianfett reviewed Oct 6, 2022

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift Outdated Show resolved Hide resolved

fabianfett reviewed Oct 6, 2022

View reviewed changes

fabianfett requested changes Oct 6, 2022

View reviewed changes

dnadoba added 2 commits October 7, 2022 18:42

Fix review comments

67a7c6d

Merge branch 'main' into dn-allow-disabling-connection-retry

06bb497

dnadoba requested a review from fabianfett October 10, 2022 09:28

fabianfett reviewed Oct 10, 2022

View reviewed changes

dnadoba added 2 commits October 10, 2022 11:50

Address review comments

8046ab3

Merge branch 'main' into dn-allow-disabling-connection-retry

6890dbd

fabianfett approved these changes Oct 10, 2022

View reviewed changes

dnadoba merged commit f17a47e into swift-server:main Oct 10, 2022

dnadoba deleted the dn-allow-disabling-connection-retry branch October 10, 2022 12:34

dnadoba restored the dn-allow-disabling-connection-retry branch October 10, 2022 12:34

dnadoba deleted the dn-allow-disabling-connection-retry branch October 10, 2022 12:35

andreasley mentioned this pull request Jun 16, 2024

Make ConnectionPool's retryConnectionEstablishment public #744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow immediate request failure on connection error #625

Allow immediate request failure on connection error #625

dnadoba commented Sep 6, 2022

fabianfett Oct 6, 2022 •

edited

Loading

dnadoba Oct 7, 2022

fabianfett Oct 10, 2022

dnadoba Oct 10, 2022

fabianfett left a comment

fabianfett Oct 6, 2022

dnadoba Oct 7, 2022

fabianfett Oct 10, 2022

dnadoba Oct 10, 2022

fabianfett Oct 10, 2022

fabianfett Oct 6, 2022

dnadoba Oct 7, 2022

dnadoba Oct 10, 2022

fabianfett left a comment

fabianfett Oct 10, 2022

fabianfett Oct 10, 2022

fabianfett left a comment

Allow immediate request failure on connection error #625

Allow immediate request failure on connection error #625

Conversation

dnadoba commented Sep 6, 2022

Motivation

Modification

Result

fabianfett Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett left a comment

Choose a reason for hiding this comment

fabianfett Oct 6, 2022 •

edited

Loading