Make ConnectionPool's retryConnectionEstablishment public
#744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alizer of `HTTPClient.Configuration.ConnectionPool`
Lukasa
reviewed
Jun 17, 2024
|
|
||
| public init(idleTimeout: TimeAmount = .seconds(60)) { | ||
| self.init(idleTimeout: idleTimeout, concurrentHTTP1ConnectionsPerHostSoftLimit: 8) | ||
| } | ||
|
|
||
| public init(idleTimeout: TimeAmount, concurrentHTTP1ConnectionsPerHostSoftLimit: Int) { | ||
| public init(idleTimeout: TimeAmount, concurrentHTTP1ConnectionsPerHostSoftLimit: Int, retryConnectionEstablishment: Bool = true) { |
There was a problem hiding this comment.
Unfortunately it's not API stable to add this extra parameter. Rather than us keep proliferating new inits, it might be nice for us to just add a zero-element initializer and encourage users towards setting the fields after default construction.
There was a problem hiding this comment.
Thanks for taking a look! I've...
- Reverted the change to the existing initializer.
- Added a parameterless initializer.
- Removed the default value for the parameter
idleTimeoutin the respective initializer (because it overlaps with the new parameterless initializer). Is that safe in terms of API stability? - Moved the default values to the respective property declarations (to prevent them having to be specified at multiple points in the initializer code). This seems cleaner to me than initializer delegation, but if you prefer a different style, let me know.
|
@swift-server-bot test this please |
Lukasa
added
the
semver/minor
… the property declarations, so they only have to be specified at one point
|
@swift-server-bot test this please |
Lukasa
reviewed
Jun 18, 2024
Co-authored-by: Cory Benfield <[email protected]>
fabianfett
approved these changes
Jun 18, 2024
|
@swift-server-bot test this please |
Lukasa
approved these changes
Jun 18, 2024
|
I'm ignoring API breakage here: it's concerned about us removing the default argument, but we've replaced it with a zero-argument constructor so it's safe from an API perspective. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, there is no way to instantly return when a connection attempt fails. The connection timeout is always awaited, regardless of the connection error. When used within a service, this behavior makes sense – network conditions may temporarily prevent establishing a connection. However, in a user-interactive environment, it's preferable to fail quickly and display the error to the user.
#625 introduced the property
retryConnectionEstablishment, which defaults totruebut can be set tofalseto not retry on connection failure. The property was added to speed up unit tests, but would also be suitable for the use-case mentioned above.This PR makes the property
retryConnectionEstablishment, which is already documented and well tested,public.This would fix #743.
The PR also cleans up some unit tests to consistently use
enableFastFailureModeForTesting()instead of settingretryConnectionEstablishmentdirectly.Questions:
networkFrameworkWaitForConnectivityalso needs to be set tofalseto enable fast fail. Would it be worthwhile to have the ability to set both properties at once?