Skip to content

add a separate configuration struct for pool #284

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 17, 2020
Merged

add a separate configuration struct for pool #284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1fa23b9
add a separate configuration struct for pool
artemredkin Jul 16, 2020
bd9fdc6
review fixes
artemredkin Jul 17, 2020
8a85fad
Merge branch 'master' into pool_configuration_refactoring
artemredkin Jul 17, 2020
ffa875a
review fix
artemredkin Jul 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Sources/AsyncHTTPClient/ConnectionPool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ class HTTP1ConnectionProvider {
case .park(let connection):
logger.trace("parking connection",
metadata: ["ahc-connection": "\(connection)"])
connection.setIdleTimeout(timeout: self.configuration.maximumAllowedIdleTimeInConnectionPool,
connection.setIdleTimeout(timeout: self.configuration.poolConfiguration.idleTimeout,
logger: self.backgroundActivityLogger)
case .closeProvider:
logger.debug("closing provider",
Expand All @@ -365,7 +365,7 @@ class HTTP1ConnectionProvider {
logger.trace("parking connection & doing further action",
metadata: ["ahc-connection": "\(connection)",
"ahc-action": "\(action)"])
connection.setIdleTimeout(timeout: self.configuration.maximumAllowedIdleTimeInConnectionPool,
connection.setIdleTimeout(timeout: self.configuration.poolConfiguration.idleTimeout,
logger: self.backgroundActivityLogger)
self.execute(action, logger: logger)
case .closeAnd(let connection, let action):
Expand Down
28 changes: 19 additions & 9 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,8 +641,8 @@ public class HTTPClient {
public var redirectConfiguration: RedirectConfiguration
/// Default client timeout, defaults to no timeouts.
public var timeout: Timeout
/// Timeout of pooled connections
public var maximumAllowedIdleTimeInConnectionPool: Optional<TimeAmount>
/// Connection pool configuration.
public var poolConfiguration: PoolConfiguration
/// Upstream proxy, defaults to no proxy.
public var proxy: Proxy?
/// Enables automatic body decompression. Supported algorithms are gzip and deflate.
Expand All @@ -653,14 +653,14 @@ public class HTTPClient {
public init(tlsConfiguration: TLSConfiguration? = nil,
redirectConfiguration: RedirectConfiguration? = nil,
timeout: Timeout = Timeout(),
maximumAllowedIdleTimeInConnectionPool: TimeAmount,
poolConfiguration: PoolConfiguration = PoolConfiguration(),
proxy: Proxy? = nil,
ignoreUncleanSSLShutdown: Bool = false,
decompression: Decompression = .disabled) {
self.tlsConfiguration = tlsConfiguration
self.redirectConfiguration = redirectConfiguration ?? RedirectConfiguration()
self.timeout = timeout
self.maximumAllowedIdleTimeInConnectionPool = maximumAllowedIdleTimeInConnectionPool
self.poolConfiguration = poolConfiguration
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
self.decompression = decompression
Expand All @@ -676,7 +676,7 @@ public class HTTPClient {
tlsConfiguration: tlsConfiguration,
redirectConfiguration: redirectConfiguration,
timeout: timeout,
maximumAllowedIdleTimeInConnectionPool: .seconds(60),
poolConfiguration: PoolConfiguration(),
proxy: proxy,
ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown,
decompression: decompression
Expand All @@ -693,7 +693,7 @@ public class HTTPClient {
self.init(tlsConfiguration: TLSConfiguration.forClient(certificateVerification: certificateVerification),
redirectConfiguration: redirectConfiguration,
timeout: timeout,
maximumAllowedIdleTimeInConnectionPool: maximumAllowedIdleTimeInConnectionPool,
poolConfiguration: PoolConfiguration(),
proxy: proxy,
ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown,
decompression: decompression)
Expand All @@ -702,15 +702,15 @@ public class HTTPClient {
public init(certificateVerification: CertificateVerification,
redirectConfiguration: RedirectConfiguration? = nil,
timeout: Timeout = Timeout(),
maximumAllowedIdleTimeInConnectionPool: TimeAmount = .seconds(60),
poolConfiguration: TimeAmount = .seconds(60),
proxy: Proxy? = nil,
ignoreUncleanSSLShutdown: Bool = false,
decompression: Decompression = .disabled,
backgroundActivityLogger: Logger?) {
self.init(tlsConfiguration: TLSConfiguration.forClient(certificateVerification: certificateVerification),
redirectConfiguration: redirectConfiguration,
timeout: timeout,
maximumAllowedIdleTimeInConnectionPool: maximumAllowedIdleTimeInConnectionPool,
poolConfiguration: PoolConfiguration(),
proxy: proxy,
ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown,
decompression: decompression)
Expand Down Expand Up @@ -811,7 +811,7 @@ public class HTTPClient {
}

extension HTTPClient.Configuration {
/// Timeout configuration
/// Timeout configuration.
public struct Timeout {
/// Specifies connect timeout.
public var connect: TimeAmount?
Expand Down Expand Up @@ -860,6 +860,16 @@ extension HTTPClient.Configuration {
/// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector.
public static func follow(max: Int, allowCycles: Bool) -> RedirectConfiguration { return .init(configuration: .follow(max: max, allowCycles: allowCycles)) }
}

/// Connection pool configuration.
public struct PoolConfiguration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this Hashable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Out of curiosity, why do you think we might need it? Also, TimeAmount is not hashable, so this will require implementing hash(into:), should I add it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

TimeAmount is Hashable as of 2.19.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As to why we need it, it's not so much that we need it as that, for trivial structures, missing Equatable and Hashable conformances tend to be minor annoyances. It's nicer just to have them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

// Specifies amount of time connections are kept idle in the pool.
var idleTimeout: TimeAmount
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, nearly missed this.

Suggested change
var idleTimeout: TimeAmount
public var idleTimeout: TimeAmount

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!


public init(idleTimeout: TimeAmount = .seconds(60)) {
self.idleTimeout = idleTimeout
}
}
}

extension ChannelPipeline {
Expand Down
4 changes: 2 additions & 2 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,7 +1742,7 @@ class HTTPClientTests: XCTestCase {

func testPoolClosesIdleConnections() {
let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
configuration: .init(maximumAllowedIdleTimeInConnectionPool: .milliseconds(100)))
configuration: .init(poolConfiguration: .init(idleTimeout: .milliseconds(100))))
defer {
XCTAssertNoThrow(try localClient.syncShutdown())
}
Expand All @@ -1753,7 +1753,7 @@ class HTTPClientTests: XCTestCase {

func testRacePoolIdleConnectionsAndGet() {
let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
configuration: .init(maximumAllowedIdleTimeInConnectionPool: .milliseconds(10)))
configuration: .init(poolConfiguration: .init(idleTimeout: .milliseconds(10))))
defer {
XCTAssertNoThrow(try localClient.syncShutdown())
}
Expand Down