don't just drop Channels on the floor if registration fails

Sources/NIO/BaseSocketChannel.swift

@@ -396,7 +396,8 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
     }
 
     deinit {
-        assert(self.lifecycleManager.canBeDestroyed, "leak of open Channel")
+        assert(self.lifecycleManager.canBeDestroyed,
+               "leak of open Channel, state: \(String(describing: self.lifecycleManager))")
     }
 
     public final func localAddress0() throws -> SocketAddress {
@@ -791,7 +792,15 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
         assert(self.eventLoop.inEventLoop)
         assert(self.isOpen)
         assert(!self.lifecycleManager.isActive)
-        register0(promise: nil)
+        let registerPromise: EventLoopPromise<Void> = self.eventLoop.newPromise()
+        register0(promise: registerPromise)
+        registerPromise.futureResult.whenFailure { (_: Error) in
+            self.close(promise: nil)
+        }
+        if let promise = promise {
+            registerPromise.futureResult.cascadeFailure(promise: promise)
+        }
+
         if self.lifecycleManager.isPreRegistered {
             try! becomeFullyRegistered0()
             if self.lifecycleManager.isRegisteredFully {

Sources/NIO/Bootstrap.swift

@@ -182,6 +182,7 @@ public final class ServerBootstrap {
                                            group: childEventLoopGroup,
                                            protocolFamily: address.protocolFamily)
         }
+
         return bind0(makeServerChannel: makeChannel) { (eventGroup, serverChannel) in
             serverChannel.registerAndDoSynchronously { serverChannel in
                 serverChannel.bind(to: address)
@@ -197,27 +198,26 @@ public final class ServerBootstrap {
         let childChannelInit = self.childChannelInit
         let childChannelOptions = self.childChannelOptions
 
-        let future: EventLoopFuture<Channel>
+        let serverChannel: ServerSocketChannel
         do {
-            let serverChannel = try makeServerChannel(eventLoop as! SelectableEventLoop, childEventLoopGroup)
-
-            future = serverChannelInit(serverChannel).then {
-                serverChannel.pipeline.add(handler: AcceptHandler(childChannelInitializer: childChannelInit,
-                                                                  childChannelOptions: childChannelOptions))
-            }.then {
-                serverChannelOptions.applyAll(channel: serverChannel)
-            }.then {
-                register(eventLoop, serverChannel)
-            }.map {
-                serverChannel
-            }
+            serverChannel = try makeServerChannel(eventLoop as! SelectableEventLoop, childEventLoopGroup)
         } catch {
-            let promise: EventLoopPromise<Channel> = eventLoop.newPromise()
-            promise.fail(error: error)
-            future = promise.futureResult
+            return eventLoop.newFailedFuture(error: error)
         }
 
-        return future
+        return serverChannelInit(serverChannel).then {
+            serverChannel.pipeline.add(handler: AcceptHandler(childChannelInitializer: childChannelInit,
+                                                              childChannelOptions: childChannelOptions))
+        }.then {
+            serverChannelOptions.applyAll(channel: serverChannel)
+        }.then {
+            register(eventLoop, serverChannel)
+        }.map {
+            serverChannel
+        }.thenIfError { error in
+            serverChannel.close0(error: error, mode: .all, promise: nil)
+            return eventLoop.newFailedFuture(error: error)
+        }
     }
 
     private class AcceptHandler: ChannelInboundHandler {
@@ -426,22 +426,26 @@ public final class ClientBootstrap {
     /// - returns: an `EventLoopFuture<Channel>` to deliver the `Channel` immediately.
     public func withConnectedSocket(descriptor: CInt) -> EventLoopFuture<Channel> {
         let eventLoop = group.next()
+        let channelInitializer = self.channelInitializer ?? { _ in eventLoop.newSucceededFuture(result: ()) }
+        let channel: SocketChannel
         do {
-            let channelInitializer = self.channelInitializer ?? { _ in eventLoop.newSucceededFuture(result: ()) }
-            let channel = try SocketChannel(eventLoop: eventLoop as! SelectableEventLoop, descriptor: descriptor)
-
-            return channelInitializer(channel).then {
-                self.channelOptions.applyAll(channel: channel)
-            }.then {
-                let promise: EventLoopPromise<Void> = eventLoop.newPromise()
-                channel.registerAlreadyConfigured0(promise: promise)
-                return promise.futureResult
-            }.map {
-                channel
-            }
+            channel = try SocketChannel(eventLoop: eventLoop as! SelectableEventLoop, descriptor: descriptor)
         } catch {
             return eventLoop.newFailedFuture(error: error)
         }
+
+        return channelInitializer(channel).then {
+            self.channelOptions.applyAll(channel: channel)
+        }.then {
+            let promise: EventLoopPromise<Void> = eventLoop.newPromise()
+            channel.registerAlreadyConfigured0(promise: promise)
+            return promise.futureResult
+        }.map {
+            channel
+        }.thenIfError { error in
+            channel.close0(error: error, mode: .all, promise: nil)
+            return channel.eventLoop.newFailedFuture(error: error)
+        }
     }
 
     private func execute(eventLoop: EventLoop,
@@ -451,20 +455,25 @@ public final class ClientBootstrap {
         let channelOptions = self.channelOptions
 
         let promise: EventLoopPromise<Channel> = eventLoop.newPromise()
+        let channel: SocketChannel
         do {
-            let channel = try SocketChannel(eventLoop: eventLoop as! SelectableEventLoop, protocolFamily: protocolFamily)
-
-            channelInitializer(channel).then {
-                channelOptions.applyAll(channel: channel)
-            }.then {
-                channel.registerAndDoSynchronously(body)
-            }.map {
-                channel
-            }.cascade(promise: promise)
+            channel = try SocketChannel(eventLoop: eventLoop as! SelectableEventLoop, protocolFamily: protocolFamily)
         } catch let err {
             promise.fail(error: err)
+            return promise.futureResult
         }
 
+        channelInitializer(channel).then {
+            channelOptions.applyAll(channel: channel)
+        }.then {
+            channel.registerAndDoSynchronously(body)
+        }.map {
+            channel
+        }.thenIfError { error in
+            channel.close0(error: error, mode: .all, promise: nil)
+            return channel.eventLoop.newFailedFuture(error: error)
+        }.cascade(promise: promise)
+
         return promise.futureResult
     }
 }
@@ -583,7 +592,7 @@ public final class DatagramBootstrap {
         }
         return bind0(makeChannel: makeChannel) { (eventLoop, channel) in
             channel.register().then {
-                _ in return channel.bind(to: address)
+                channel.bind(to: address)
             }
         }
     }
@@ -593,24 +602,22 @@ public final class DatagramBootstrap {
         let channelInitializer = self.channelInitializer ?? { _ in eventLoop.newSucceededFuture(result: ()) }
         let channelOptions = self.channelOptions
 
-        let future: EventLoopFuture<Channel>
+        let channel: DatagramChannel
         do {
-            let channel = try makeChannel(eventLoop as! SelectableEventLoop)
-
-            future = channelInitializer(channel).then {
-                channelOptions.applyAll(channel: channel)
-            }.then {
-                registerAndBind(eventLoop, channel)
-            }.map {
-                channel
-            }
+            channel = try makeChannel(eventLoop as! SelectableEventLoop)
         } catch {
-            let promise: EventLoopPromise<Channel> = eventLoop.newPromise()
-            promise.fail(error: error)
-            future = promise.futureResult
+            return eventLoop.newFailedFuture(error: error)
         }
 
-        return future
+        return channelInitializer(channel).then {
+            channelOptions.applyAll(channel: channel)
+        }.then {
+            registerAndBind(eventLoop, channel)
+        }.map {
+            channel
+        }.thenIfError { error in
+            eventLoop.newFailedFuture(error: error)
+        }
     }
 }
 

Tests/NIOTests/ChannelTests+XCTest.swift

@@ -71,6 +71,10 @@ extension ChannelTests {
                 ("testCloseInUnregister", testCloseInUnregister),
                 ("testLazyRegistrationWorksForServerSockets", testLazyRegistrationWorksForServerSockets),
                 ("testLazyRegistrationWorksForClientSockets", testLazyRegistrationWorksForClientSockets),
+                ("testFailedRegistrationOfClientSocket", testFailedRegistrationOfClientSocket),
+                ("testFailedRegistrationOfAcceptedSocket", testFailedRegistrationOfAcceptedSocket),
+                ("testFailedRegistrationOfServerSocket", testFailedRegistrationOfServerSocket),
+                ("testTryingToBindOnPortThatIsAlreadyBoundFailsButDoesNotCrash", testTryingToBindOnPortThatIsAlreadyBoundFailsButDoesNotCrash),
            ]
    }
 }

Tests/NIOTests/ChannelTests.swift

@@ -2360,6 +2360,106 @@ public class ChannelTests: XCTestCase {
         XCTAssertTrue(client.isActive)
         XCTAssertEqual(serverChannel.localAddress!, client.remoteAddress!)
     }
+
+    func testFailedRegistrationOfClientSocket() throws {
+        let group = MultiThreadedEventLoopGroup(numThreads: 2)
+        defer {
+            XCTAssertNoThrow(try group.syncShutdownGracefully())
+        }
+        let serverChannel = try ServerBootstrap(group: group).bind(host: "localhost", port: 0).wait()
+        defer {
+            XCTAssertNoThrow(try serverChannel.close().wait())
+        }
+        do {
+            let clientChannel = try ClientBootstrap(group: group)
+                .channelInitializer { channel in
+                    channel.pipeline.add(handler: FailRegistrationHandler())
+                }
+                .connect(to: serverChannel.localAddress!)
+                .wait()
+            XCTFail("shouldn't have reached this but got \(clientChannel)")
+        } catch FailRegistrationHandler.RegistrationFailedError.error {
+            // ok
+        } catch {
+            XCTFail("unexpected error \(error)")
+        }
+    }
+
+    func testFailedRegistrationOfAcceptedSocket() throws {
+        let group = MultiThreadedEventLoopGroup(numThreads: 2)
+        defer {
+            XCTAssertNoThrow(try group.syncShutdownGracefully())
+        }
+        let serverChannel = try ServerBootstrap(group: group)
+            .childChannelInitializer { channel in
+                channel.pipeline.add(handler: FailRegistrationHandler())
+            }
+            .bind(host: "localhost", port: 0).wait()
+        defer {
+            XCTAssertNoThrow(try serverChannel.close().wait())
+        }
+        let clientChannel = try ClientBootstrap(group: group)
+            .connect(to: serverChannel.localAddress!)
+            .wait()
+        try clientChannel.closeFuture.wait()
+    }
+
+    func testFailedRegistrationOfServerSocket() throws {
+        let group = MultiThreadedEventLoopGroup(numThreads: 2)
+        defer {
+            XCTAssertNoThrow(try group.syncShutdownGracefully())
+        }
+        do {
+            let serverChannel = try ServerBootstrap(group: group)
+                .serverChannelInitializer { channel in
+                    channel.pipeline.add(handler: FailRegistrationHandler())
+                }
+                .bind(host: "localhost", port: 0).wait()
+            XCTFail("shouldn't be reached")
+            XCTAssertNoThrow(try serverChannel.close().wait())
+        } catch FailRegistrationHandler.RegistrationFailedError.error {
+            // ok
+        } catch {
+            XCTFail("unexpected error \(error)")
+        }
+    }
+
+    func testTryingToBindOnPortThatIsAlreadyBoundFailsButDoesNotCrash() throws {
+        // this is a regression test for #417
+
+        let group = MultiThreadedEventLoopGroup(numThreads: 1)
+        defer {
+            XCTAssertNoThrow(try group.syncShutdownGracefully())
+        }
+
+        let serverChannel1 = try! ServerBootstrap(group: group)
+            .bind(host: "localhost", port: 0)
+            .wait()
+        defer {
+            XCTAssertNoThrow(try serverChannel1.close().wait())
+        }
+
+        do {
+            let serverChannel2 = try ServerBootstrap(group: group)
+                .bind(to: serverChannel1.localAddress!)
+                .wait()
+            XCTFail("shouldn't have succeeded, got two server channels on the same port: \(serverChannel1) and \(serverChannel2)")
+        } catch let e as IOError where e.errnoCode == EADDRINUSE {
+            // OK
+        } catch {
+            XCTFail("unexpected error: \(error)")
+        }
+    }
+}
+
+fileprivate final class FailRegistrationHandler: ChannelOutboundHandler {
+    enum RegistrationFailedError: Error { case error }
+
+    typealias OutboundIn = Never
+
+    func register(ctx: ChannelHandlerContext, promise: EventLoopPromise<Void>?) {
+        promise!.fail(error: RegistrationFailedError.error)
+    }
 }
 
 fileprivate class VerifyConnectionFailureHandler: ChannelInboundHandler {

docker/docker-compose.1404.41.yaml

@@ -17,7 +17,7 @@ services:
     image: swift-nio:14.04-4.1
     environment:
       - MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=47000
-      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=776100
+      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=792100
       - MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4600
       - MAX_ALLOCS_ALLOWED_bytebuffer_lots_of_rw=3100
       - MAX_ALLOCS_ALLOWED_future_lots_of_callbacks=99100
@@ -26,7 +26,7 @@ services:
     image: swift-nio:14.04-4.1
     environment:
       - MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=47000
-      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=776100
+      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=792100
       - MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4600
       - MAX_ALLOCS_ALLOWED_bytebuffer_lots_of_rw=3100
       - MAX_ALLOCS_ALLOWED_future_lots_of_callbacks=99100

docker/docker-compose.1604.41.yaml

@@ -16,7 +16,7 @@ services:
     image: swift-nio:16.04-4.1
     environment:
       - MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=47000
-      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=777100
+      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=793100
       - MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4600
       - MAX_ALLOCS_ALLOWED_bytebuffer_lots_of_rw=3100
       - MAX_ALLOCS_ALLOWED_future_lots_of_callbacks=99100
@@ -25,7 +25,7 @@ services:
     image: swift-nio:16.04-4.1
     environment:
       - MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=47000
-      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=777100
+      - MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=793100
       - MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4600
       - MAX_ALLOCS_ALLOWED_bytebuffer_lots_of_rw=3100
       - MAX_ALLOCS_ALLOWED_future_lots_of_callbacks=99100