From d05001064f8efcbd4dcffb706fdbc1eeb409e0c5 Mon Sep 17 00:00:00 2001 From: Gus Cairo Date: Mon, 16 Dec 2024 15:07:44 +0000 Subject: [PATCH 1/4] Wait for closeFuture instead of close promise in executeThenClose --- Sources/NIOCore/AsyncChannel/AsyncChannel.swift | 16 +++++++--------- Sources/NIOCore/Channel.swift | 5 +++++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/Sources/NIOCore/AsyncChannel/AsyncChannel.swift b/Sources/NIOCore/AsyncChannel/AsyncChannel.swift index 10354f0760..74fdd53dcb 100644 --- a/Sources/NIOCore/AsyncChannel/AsyncChannel.swift +++ b/Sources/NIOCore/AsyncChannel/AsyncChannel.swift @@ -301,15 +301,13 @@ public struct NIOAsyncChannel: Sendable { } } - do { - self._outbound.finish() - try await self.channel.close().get() - } catch { - if let error = error as? ChannelError, error == .alreadyClosed { - return result - } - throw error - } + self._outbound.finish() + // We ignore errors from close, since all we care about is that the channel has been closed + // at this point. + self.channel.close(promise: nil) + // `closeFuture` is never failed, so we can ignore the error + try? await self.channel.closeFuture.get() + return result } } diff --git a/Sources/NIOCore/Channel.swift b/Sources/NIOCore/Channel.swift index 8d0af1fec5..227efce5ce 100644 --- a/Sources/NIOCore/Channel.swift +++ b/Sources/NIOCore/Channel.swift @@ -107,6 +107,11 @@ public protocol Channel: AnyObject, ChannelOutboundInvoker, _NIOPreconcurrencySe var allocator: ByteBufferAllocator { get } /// The `closeFuture` will fire when the `Channel` has been closed. + /// + /// - Important: This future will never be failed, as it signals when the channel has been closed, and this action cannot fail. + /// If you are interested in any errors thrown during `close` to diagnose any unclean channel closures, you + /// should instead use the future returned from ``close(mode:file:line:)`` or pass a promise via + /// ``close(mode:promise:)``. var closeFuture: EventLoopFuture { get } /// The `ChannelPipeline` which handles all I/O events and requests associated with this `Channel`. From e57de8087f02ac7ae8fd99e5b6e8abd3f8075cc3 Mon Sep 17 00:00:00 2001 From: Gus Cairo Date: Tue, 17 Dec 2024 10:48:24 +0000 Subject: [PATCH 2/4] Try to fix docs --- Sources/NIOCore/Channel.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/NIOCore/Channel.swift b/Sources/NIOCore/Channel.swift index 227efce5ce..9682adf10a 100644 --- a/Sources/NIOCore/Channel.swift +++ b/Sources/NIOCore/Channel.swift @@ -110,8 +110,8 @@ public protocol Channel: AnyObject, ChannelOutboundInvoker, _NIOPreconcurrencySe /// /// - Important: This future will never be failed, as it signals when the channel has been closed, and this action cannot fail. /// If you are interested in any errors thrown during `close` to diagnose any unclean channel closures, you - /// should instead use the future returned from ``close(mode:file:line:)`` or pass a promise via - /// ``close(mode:promise:)``. + /// should instead use the future returned from ``Channel/close(mode:file:line:)`` or pass a promise via + /// ``Channel/close(mode:promise:)``. var closeFuture: EventLoopFuture { get } /// The `ChannelPipeline` which handles all I/O events and requests associated with this `Channel`. From 7ee9d27d027d1bbb5a6eceef742fac791509b607 Mon Sep 17 00:00:00 2001 From: Gus Cairo Date: Tue, 17 Dec 2024 14:48:31 +0000 Subject: [PATCH 3/4] Fix up docs --- Sources/NIOCore/Channel.swift | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Sources/NIOCore/Channel.swift b/Sources/NIOCore/Channel.swift index 9682adf10a..9793102d5e 100644 --- a/Sources/NIOCore/Channel.swift +++ b/Sources/NIOCore/Channel.swift @@ -108,10 +108,11 @@ public protocol Channel: AnyObject, ChannelOutboundInvoker, _NIOPreconcurrencySe /// The `closeFuture` will fire when the `Channel` has been closed. /// - /// - Important: This future will never be failed, as it signals when the channel has been closed, and this action cannot fail. + /// - Important: This future should never be failed: it signals when the channel has been closed, and this action should not fail, + /// regardless of whether the close happenned cleanly or not. /// If you are interested in any errors thrown during `close` to diagnose any unclean channel closures, you - /// should instead use the future returned from ``Channel/close(mode:file:line:)`` or pass a promise via - /// ``Channel/close(mode:promise:)``. + /// should instead use the future returned from ``ChannelOutboundInvoker/close(mode:file:line:)-7hlgf`` + /// or pass a promise via ``ChannelOutboundInvoker/close(mode:promise:)``. var closeFuture: EventLoopFuture { get } /// The `ChannelPipeline` which handles all I/O events and requests associated with this `Channel`. From 01c568cf71160b1c42b31d2eb364b398750e8885 Mon Sep 17 00:00:00 2001 From: Gus Cairo Date: Tue, 17 Dec 2024 16:00:51 +0000 Subject: [PATCH 4/4] Add an assertion failure if the channel's closeFuture is failed --- Sources/NIOCore/AsyncChannel/AsyncChannel.swift | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Sources/NIOCore/AsyncChannel/AsyncChannel.swift b/Sources/NIOCore/AsyncChannel/AsyncChannel.swift index 74fdd53dcb..105862576d 100644 --- a/Sources/NIOCore/AsyncChannel/AsyncChannel.swift +++ b/Sources/NIOCore/AsyncChannel/AsyncChannel.swift @@ -305,8 +305,20 @@ public struct NIOAsyncChannel: Sendable { // We ignore errors from close, since all we care about is that the channel has been closed // at this point. self.channel.close(promise: nil) - // `closeFuture` is never failed, so we can ignore the error - try? await self.channel.closeFuture.get() + // `closeFuture` should never be failed, so we could ignore the error. However, do an + // assertionFailure to guide bad Channel implementations that are incorrectly failing this + // future to stop failing it. + do { + try await self.channel.closeFuture.get() + } catch { + assertionFailure( + """ + The channel's closeFuture should never be failed, but it was failed with error: \(error). + This is an error in the channel's implementation. + Refer to `Channel/closeFuture`'s documentation for more information. + """ + ) + } return result }