From 42189473b77120eba8fc18020b6a251b48cf0890 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 13 Mar 2023 11:23:36 +0000 Subject: [PATCH 1/3] Fix crash if connection is closed very early If the channel is closed before flatMap is executed, all ChannelHandler are removed and `TLSEventsHandler` is therefore not present either. We need to tolerate this even though it is very rare. Testing ideas welcome. Fixes #670 --- .../HTTPConnectionPool+Factory.swift | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift index e94e967a6..10668a011 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift @@ -361,14 +361,19 @@ extension HTTPConnectionPool.ConnectionFactory { var channelFuture = bootstrapFuture.flatMap { bootstrap -> EventLoopFuture in return bootstrap.connect(target: self.key.connectionTarget) }.flatMap { channel -> EventLoopFuture<(Channel, String?)> in - // It is save to use `try!` here, since we are sure, that a `TLSEventsHandler` exists - // within the pipeline. It is added in `makeTLSBootstrap`. - let tlsEventHandler = try! channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self) - - // The tlsEstablishedFuture is set as soon as the TLSEventsHandler is in a - // pipeline. It is created in TLSEventsHandler's handlerAdded method. - return tlsEventHandler.tlsEstablishedFuture!.flatMap { negotiated in - channel.pipeline.removeHandler(tlsEventHandler).map { (channel, negotiated) } + do { + // if the channel is closed before flatMap is executed, all ChannelHandler are removed + // and TLSEventsHandler is therefore not present either + let tlsEventHandler = try channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self) + + // The tlsEstablishedFuture is set as soon as the TLSEventsHandler is in a + // pipeline. It is created in TLSEventsHandler's handlerAdded method. + return tlsEventHandler.tlsEstablishedFuture!.flatMap { negotiated in + channel.pipeline.removeHandler(tlsEventHandler).map { (channel, negotiated) } + } + } catch { + precondition(channel.isActive == false, "if the channel is still active then TLSEventsHandler must be present but got error \(error)") + return channel.eventLoop.makeFailedFuture(HTTPClientError.remoteConnectionClosed) } } From d0d1cee09e56b1c34758b270a5da99114903b6cc Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 13 Mar 2023 12:59:34 +0000 Subject: [PATCH 2/3] drop precondition to assert --- .../ConnectionPool/HTTPConnectionPool+Factory.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift index 10668a011..5be6c846e 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift @@ -372,7 +372,7 @@ extension HTTPConnectionPool.ConnectionFactory { channel.pipeline.removeHandler(tlsEventHandler).map { (channel, negotiated) } } } catch { - precondition(channel.isActive == false, "if the channel is still active then TLSEventsHandler must be present but got error \(error)") + assert(channel.isActive == false, "if the channel is still active then TLSEventsHandler must be present but got error \(error)") return channel.eventLoop.makeFailedFuture(HTTPClientError.remoteConnectionClosed) } } From 1ea20b35ca0c610d0fd883bf994dbef09d734702 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 14 Mar 2023 10:31:19 +0000 Subject: [PATCH 3/3] run SwiftFormat --- .../ConnectionPool/HTTPConnectionPool+Factory.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift index 5be6c846e..60338f615 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift @@ -365,7 +365,7 @@ extension HTTPConnectionPool.ConnectionFactory { // if the channel is closed before flatMap is executed, all ChannelHandler are removed // and TLSEventsHandler is therefore not present either let tlsEventHandler = try channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self) - + // The tlsEstablishedFuture is set as soon as the TLSEventsHandler is in a // pipeline. It is created in TLSEventsHandler's handlerAdded method. return tlsEventHandler.tlsEstablishedFuture!.flatMap { negotiated in