From 63e9a46f34c454a95acd340eb578ac5e36ef11d2 Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Thu, 13 May 2021 14:43:38 +0100 Subject: [PATCH] SSLContextCache: use DispatchQueue instead of NIOThreadPool Motivation: In the vast majority of cases, we'll only ever create one and only one `NIOSSLContext`. It's therefore wasteful to keep around a whole thread doing nothing just for that. A `DispatchQueue` is absolutely fine here. Modification: Run the `NIOSSLContext` creation on a `DispatchQueue` instead. Result: Fewer threads hanging around. --- Sources/AsyncHTTPClient/ConnectionPool.swift | 2 - Sources/AsyncHTTPClient/SSLContextCache.swift | 53 ++----------------- .../SSLContextCacheTests+XCTest.swift | 3 +- .../SSLContextCacheTests.swift | 32 ++++++----- 4 files changed, 22 insertions(+), 68 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index 4aecf6d17..8845f9709 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -109,8 +109,6 @@ final class ConnectionPool { self.providers.values } - self.sslContextCache.shutdown() - return EventLoopFuture.reduce(true, providers.map { $0.close() }, on: eventLoop) { $0 && $1 } } diff --git a/Sources/AsyncHTTPClient/SSLContextCache.swift b/Sources/AsyncHTTPClient/SSLContextCache.swift index 582d2cee8..005c475de 100644 --- a/Sources/AsyncHTTPClient/SSLContextCache.swift +++ b/Sources/AsyncHTTPClient/SSLContextCache.swift @@ -12,69 +12,22 @@ // //===----------------------------------------------------------------------===// +import Dispatch import Logging import NIO import NIOConcurrencyHelpers import NIOSSL class SSLContextCache { - private var state = State.activeNoThread private let lock = Lock() private var sslContextCache = LRUCache() - private let threadPool = NIOThreadPool(numberOfThreads: 1) - - enum State { - case activeNoThread - case active - case shutDown - } - - init() {} - - func shutdown() { - self.lock.withLock { () -> Void in - switch self.state { - case .activeNoThread: - self.state = .shutDown - case .active: - self.state = .shutDown - self.threadPool.shutdownGracefully { maybeError in - precondition(maybeError == nil, "\(maybeError!)") - } - case .shutDown: - preconditionFailure("SSLContextCache shut down twice") - } - } - } - - deinit { - assert(self.state == .shutDown) - } + private let offloadQueue = DispatchQueue(label: "io.github.swift-server.AsyncHTTPClient.SSLContextCache") } extension SSLContextCache { - private struct SSLContextCacheShutdownError: Error {} - func sslContext(tlsConfiguration: TLSConfiguration, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture { - let earlyExitError: Error? = self.lock.withLock { () -> Error? in - switch self.state { - case .activeNoThread: - self.state = .active - self.threadPool.start() - return nil - case .active: - return nil - case .shutDown: - return SSLContextCacheShutdownError() - } - } - - if let error = earlyExitError { - return eventLoop.makeFailedFuture(error) - } - let eqTLSConfiguration = BestEffortHashableTLSConfiguration(wrapping: tlsConfiguration) let sslContext = self.lock.withLock { self.sslContextCache.find(key: eqTLSConfiguration) @@ -88,7 +41,7 @@ extension SSLContextCache { logger.debug("creating new SSL context", metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) - let newSSLContext = self.threadPool.runIfActive(eventLoop: eventLoop) { + let newSSLContext = self.offloadQueue.asyncWithFuture(eventLoop: eventLoop) { try NIOSSLContext(configuration: tlsConfiguration) } diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift index 78a2d5e6f..d98f5a853 100644 --- a/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift @@ -25,10 +25,9 @@ import XCTest extension SSLContextCacheTests { static var allTests: [(String, (SSLContextCacheTests) -> () throws -> Void)] { return [ - ("testJustStartingAndStoppingAContextCacheWorks", testJustStartingAndStoppingAContextCacheWorks), ("testRequestingSSLContextWorks", testRequestingSSLContextWorks), - ("testRequestingSSLContextAfterShutdownThrows", testRequestingSSLContextAfterShutdownThrows), ("testCacheWorks", testCacheWorks), + ("testCacheDoesNotReturnWrongEntry", testCacheDoesNotReturnWrongEntry), ] } } diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift index 980e7f94a..d44d65432 100644 --- a/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift @@ -18,17 +18,12 @@ import NIOSSL import XCTest final class SSLContextCacheTests: XCTestCase { - func testJustStartingAndStoppingAContextCacheWorks() { - SSLContextCache().shutdown() - } - func testRequestingSSLContextWorks() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) - cache.shutdown() } XCTAssertNoThrow(try cache.sslContext(tlsConfiguration: .forClient(), @@ -36,7 +31,7 @@ final class SSLContextCacheTests: XCTestCase { logger: HTTPClient.loggingDisabled).wait()) } - func testRequestingSSLContextAfterShutdownThrows() { + func testCacheWorks() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() @@ -44,19 +39,26 @@ final class SSLContextCacheTests: XCTestCase { XCTAssertNoThrow(try group.syncShutdownGracefully()) } - cache.shutdown() - XCTAssertThrowsError(try cache.sslContext(tlsConfiguration: .forClient(), - eventLoop: eventLoop, - logger: HTTPClient.loggingDisabled).wait()) + var firstContext: NIOSSLContext? + var secondContext: NIOSSLContext? + + XCTAssertNoThrow(firstContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNotNil(firstContext) + XCTAssertNotNil(secondContext) + XCTAssert(firstContext === secondContext) } - func testCacheWorks() { + func testCacheDoesNotReturnWrongEntry() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) - cache.shutdown() } var firstContext: NIOSSLContext? @@ -65,11 +67,13 @@ final class SSLContextCacheTests: XCTestCase { XCTAssertNoThrow(firstContext = try cache.sslContext(tlsConfiguration: .forClient(), eventLoop: eventLoop, logger: HTTPClient.loggingDisabled).wait()) - XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(), + + // Second one has a _different_ TLSConfiguration. + XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(certificateVerification: .none), eventLoop: eventLoop, logger: HTTPClient.loggingDisabled).wait()) XCTAssertNotNil(firstContext) XCTAssertNotNil(secondContext) - XCTAssert(firstContext === secondContext) + XCTAssert(firstContext !== secondContext) } }