fix event loop hop between registration and activation of accepted ch…

…annels (#389)

Motivation:

Once again, we had an extra event loop hop between a channel
registration and its activation. Usually this shows up as `EPOLLHUP` but
not so for accepted channels. What happened instead is that we had a
small race window after we accepted a channel. It was in a state where
it was not marked active _yet_ and therefore we'd not read out its data
in case we received a `.readEOF`. That usually leads to a stale
connection. Fortunately it doesn't happen very often that the client
connects, immediately sends some data and then shuts the write end of
the socket.

Modifications:

prevent the event loop hop between registration and activation

Result:

will always read out the read buffer on .readEOF

IntegrationTests/tests_01_http/test_23_repeated_reqs_with_half_closure.sh

@@ -0,0 +1,26 @@
+#!/bin/bash
+##===----------------------------------------------------------------------===##
+##
+## This source file is part of the SwiftNIO open source project
+##
+## Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
+## Licensed under Apache License v2.0
+##
+## See LICENSE.txt for license information
+## See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+##
+## SPDX-License-Identifier: Apache-2.0
+##
+##===----------------------------------------------------------------------===##
+
+source defines.sh
+
+token=$(create_token)
+start_server "$token"
+socket=$(get_socket "$token")
+echo -ne 'HTTP/1.1 200 OK\r\ncontent-length: 12\r\n\r\nHello World!' > "$tmp/expected"
+for f in $(seq 2000); do
+    echo -e 'GET / HTTP/1.1\r\n\r\n' | nc -w10 -U "$socket" > "$tmp/actual"
+    assert_equal_files "$tmp/expected" "$tmp/actual"
+done
+stop_server "$token"

Sources/NIO/BaseSocketChannel.swift

@@ -808,6 +808,11 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
         assert(!self.lifecycleManager.hasSeenEOFNotification)
         self.lifecycleManager.hasSeenEOFNotification = true
 
+        // we can't be not active but still registered here; this would mean that we got a notification about a
+        // channel before we're ready to receive them.
+        assert(self.lifecycleManager.isActive || !self.lifecycleManager.isRegistered,
+               "illegal state: active: \(self.lifecycleManager.isActive), registered: \(self.lifecycleManager.isRegistered)")
+
         self.readEOF0()
 
         assert(!self.interestedEvent.contains(.read))

Sources/NIO/SocketChannel.swift

@@ -453,13 +453,15 @@ final class ServerSocketChannel: BaseSocketChannel<ServerSocket> {
         assert(eventLoop.inEventLoop)
 
         let ch = data.forceAsOther() as SocketChannel
-        ch.register().thenThrowing {
-            guard ch.isOpen else {
-                throw ChannelError.ioOnClosedChannel
+        ch.eventLoop.execute {
+            ch.register().thenThrowing {
+                guard ch.isOpen else {
+                    throw ChannelError.ioOnClosedChannel
+                }
+                ch.becomeActive0(promise: nil)
+            }.whenFailure { error in
+                ch.close(promise: nil)
             }
-            ch.becomeActive0(promise: nil)
-        }.whenFailure { error in
-            ch.close(promise: nil)
         }
     }
 

Tests/NIOTests/EventLoopTest.swift

@@ -126,16 +126,23 @@ public class EventLoopTest : XCTestCase {
         // to cleanly shut down all the channels before it actually closes. We add a custom channel that we can use
         // to wedge the event loop in the "shutting down" state, ensuring that we have plenty of time to attempt the
         // registration.
-        class WedgeOpenHandler: ChannelOutboundHandler {
+        class WedgeOpenHandler: ChannelDuplexHandler {
+            typealias InboundIn = Any
             typealias OutboundIn = Any
             typealias OutboundOut = Any
 
             private let promiseRegisterCallback: (EventLoopPromise<Void>) -> Void
 
             var closePromise: EventLoopPromise<Void>? = nil
+            private let channelActivePromise: EventLoopPromise<Void>?
 
-            init(_ promiseRegisterCallback: @escaping (EventLoopPromise<Void>) -> Void) {
+            init(channelActivePromise: EventLoopPromise<Void>? = nil, _ promiseRegisterCallback: @escaping (EventLoopPromise<Void>) -> Void) {
                 self.promiseRegisterCallback = promiseRegisterCallback
+                self.channelActivePromise = channelActivePromise
+            }
+
+            func channelActive(ctx: ChannelHandlerContext) {
+                self.channelActivePromise?.succeed(result: ())
             }
 
             func close(ctx: ChannelHandlerContext, mode: CloseMode, promise: EventLoopPromise<Void>?) {
@@ -167,9 +174,10 @@ public class EventLoopTest : XCTestCase {
         }
         let loop = group.next() as! SelectableEventLoop
 
+        let serverChannelUp: EventLoopPromise<Void> = group.next().newPromise()
         let serverChannel = try ServerBootstrap(group: group)
             .childChannelInitializer { channel in
-                channel.pipeline.add(handler: WedgeOpenHandler { promise in
+                channel.pipeline.add(handler: WedgeOpenHandler(channelActivePromise: serverChannelUp) { promise in
                     promiseQueue.sync { promises.append(promise) }
                 })
             }
@@ -196,6 +204,8 @@ public class EventLoopTest : XCTestCase {
         // Wait for the connect to complete.
         XCTAssertNoThrow(try connectPromise.futureResult.wait())
 
+        XCTAssertNoThrow(try serverChannelUp.futureResult.wait())
+
         // Now we're going to start closing the event loop. This should not immediately succeed.
         let loopCloseFut = loop.closeGently()