don't crash if close is called when close fails (#387)

Motivation:

@vlm hit an interesting situation which is very likely the sign of
another bug that we have yet to find:

During a close, @vlm hit an error in close which lead to a user callout
which then closed again.

The immediate fix is simple (this PR) is as always not to call out to
the user before reconciling our own state. But hitting this bug also
means that either a `socket.close` or a `selectableEventLoop.deregister`
failed as we only called out in those situations. That definitely hides
a condition that is hard to imagine without another bug that we still
need to find.

Modifications:

in `BaseSocketChannel.close0` follow rule 0 and don't call out before
reconciling state.

Result:

fewer crashes, less potential to hide other bugs.

Sources/NIO/BaseSocket.swift

@@ -394,7 +394,7 @@ class BaseSocket: Selectable {
     /// After the socket was closed all other methods will throw an `IOError` when called.
     ///
     /// - throws: An `IOError` if the operation failed.
-    final func close() throws {
+    func close() throws {
         try withUnsafeFileDescriptor { fd in
             try Posix.close(descriptor: fd)
         }

Sources/NIO/BaseSocketChannel.swift

@@ -664,33 +664,51 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
             return
         }
 
+        // === BEGIN: No user callouts ===
+
+        // this is to register all error callouts as all the callouts must happen after we transition out state
+        var errorCallouts: [(ChannelPipeline) -> Void] = []
+
         self.interestedEvent = .reset
         do {
             try selectableEventLoop.deregister(channel: self)
         } catch let err {
+            errorCallouts.append { pipeline in
                 pipeline.fireErrorCaught0(error: err)
             }
+        }
 
         let p: EventLoopPromise<Void>?
         do {
             try socket.close()
             p = promise
         } catch {
+            errorCallouts.append { (_: ChannelPipeline) in
                 promise?.fail(error: error)
                 // Set p to nil as we want to ensure we pass nil to becomeInactive0(...) so we not try to notify the promise again.
+            }
             p = nil
         }
 
         // Transition our internal state.
         let callouts = self.lifecycleManager.close(promise: p)
 
+        // === END: No user callouts (now that our state is reconciled, we can call out to user code.) ===
+
+        // this must be the first to call out as it transitions the PendingWritesManager into the closed state
+        // and we assert elsewhere that the PendingWritesManager has the same idea of 'open' as we have in here.
+        self.cancelWritesOnClose(error: error)
+
+        // this should be a no-op as we shouldn't have any
+        errorCallouts.forEach {
+            $0(self.pipeline)
+        }
+
         if let connectPromise = self.pendingConnect {
             self.pendingConnect = nil
             connectPromise.fail(error: error)
         }
 
-        // Now that our state is sensible, we can call out to user code.
-        self.cancelWritesOnClose(error: error)
         callouts(self.pipeline)
 
         eventLoop.execute {

Tests/NIOTests/ChannelTests+XCTest.swift

@@ -68,6 +68,7 @@ extension ChannelTests {
                 ("testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash", testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash),
                 ("testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown", testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown),
                 ("testConnectWithECONNREFUSEDGetsTheRightError", testConnectWithECONNREFUSEDGetsTheRightError),
+                ("testCloseInUnregister", testCloseInUnregister),
            ]
    }
 }

Tests/NIOTests/ChannelTests.swift

@@ -2263,6 +2263,60 @@ public class ChannelTests: XCTestCase {
         }
     }
 
+    func testCloseInUnregister() throws {
+        enum DummyError: Error { case dummy }
+        class SocketFailingClose: Socket {
+            init() throws {
+                try super.init(protocolFamily: PF_INET, type: Posix.SOCK_STREAM, setNonBlocking: true)
+            }
+
+            override func close() throws {
+                _ = try? super.close()
+                throw DummyError.dummy
+            }
+        }
+
+        let group = MultiThreadedEventLoopGroup(numThreads: 2)
+        defer {
+            XCTAssertNoThrow(try group.syncShutdownGracefully())
+        }
+        let sc = try SocketChannel(socket: SocketFailingClose(), eventLoop: group.next() as! SelectableEventLoop)
+
+        let serverChannel = try ServerBootstrap(group: group.next())
+            .bind(host: "127.0.0.1", port: 0)
+            .wait()
+        defer {
+            XCTAssertNoThrow(try serverChannel.syncCloseAcceptingAlreadyClosed())
+        }
+
+        XCTAssertNoThrow(try sc.eventLoop.submit {
+            sc.register().then {
+                sc.connect(to: serverChannel.localAddress!)
+            }
+        }.wait().wait() as Void)
+
+        do {
+            try sc.eventLoop.submit { () -> EventLoopFuture<Void> in
+                let p: EventLoopPromise<Void> = sc.eventLoop.newPromise()
+                // this callback must be attached before we call the close
+                let f = p.futureResult.map {
+                    XCTFail("shouldn't be reached")
+                }.thenIfError { err in
+                    XCTAssertNotNil(err as? DummyError)
+                    return sc.close()
+                }
+                sc.close(promise: p)
+                return f
+            }.wait().wait()
+            XCTFail("shouldn't be reached")
+        } catch ChannelError.alreadyClosed {
+            // ok
+        } catch {
+            XCTFail("wrong error: \(error)")
+        }
+
+    }
+
 }
 
 fileprivate class VerifyConnectionFailureHandler: ChannelInboundHandler {