Skip to content
Permalink
Browse files

AcceptHandler: don't swallow close errors when quiescing (#1015)

Motivation:

When receiving the ChannelShouldQuiesceEvent event, the AcceptHandler
correctly closes the ServerChannel. However, if that fails, it swallows
the error which is incorrect.

Modifications:

Don't swallow the error, send it through the pipeline.

Result:

More correct.
  • Loading branch information...
weissi authored and Lukasa committed May 24, 2019
1 parent 0357118 commit ca9278670fa0dbe99242efb6f053a8d1ad56752c
Showing with 48 additions and 2 deletions.
  1. +1 −1 Package.swift
  2. +3 −1 Sources/NIO/Bootstrap.swift
  3. +1 −0 Tests/NIOTests/ChannelTests+XCTest.swift
  4. +43 −0 Tests/NIOTests/ChannelTests.swift
@@ -62,7 +62,7 @@ var targets: [PackageDescription.Target] = [
.target(name: "NIOTestUtils",
dependencies: ["NIO"]),
.testTarget(name: "NIOTests",
dependencies: ["NIO", "NIOFoundationCompat"]),
dependencies: ["NIO", "NIOFoundationCompat", "NIOTestUtils", "NIOConcurrencyHelpers"]),
.testTarget(name: "NIOConcurrencyHelpersTests",
dependencies: ["NIOConcurrencyHelpers"]),
.testTarget(name: "NIOHTTP1Tests",
@@ -249,7 +249,9 @@ public final class ServerBootstrap {

func userInboundEventTriggered(context: ChannelHandlerContext, event: Any) {
if event is ChannelShouldQuiesceEvent {
context.channel.close(promise: nil)
context.channel.close().whenFailure { error in
context.fireErrorCaught(error)
}
}
context.fireUserInboundEventTriggered(event)
}
@@ -77,6 +77,7 @@ extension ChannelTests {
("testCloseInReadTriggeredByDrainingTheReceiveBufferBecauseOfWriteError", testCloseInReadTriggeredByDrainingTheReceiveBufferBecauseOfWriteError),
("testApplyingTwoDistinctSocketOptionsOfSameTypeWorks", testApplyingTwoDistinctSocketOptionsOfSameTypeWorks),
("testUnprocessedOutboundUserEventFailsOnServerSocketChannel", testUnprocessedOutboundUserEventFailsOnServerSocketChannel),
("testAcceptHandlerDoesNotSwallowCloseErrorsWhenQuiescing", testAcceptHandlerDoesNotSwallowCloseErrorsWhenQuiescing),
]
}
}
@@ -15,6 +15,7 @@
import XCTest
@testable import NIO
import NIOConcurrencyHelpers
import NIOTestUtils
import Dispatch

class ChannelLifecycleHandler: ChannelInboundHandler {
@@ -2671,6 +2672,48 @@ public final class ChannelTests: XCTestCase {
}
}
}

func testAcceptHandlerDoesNotSwallowCloseErrorsWhenQuiescing() {
// AcceptHandler is a `private class` so I can only implicitly get it by creating the real thing
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
defer {
XCTAssertNoThrow(try group.syncShutdownGracefully())
}

let counter = EventCounterHandler()

struct DummyError: Error {}
class MakeFirstCloseFailAndDontActuallyCloseHandler: ChannelOutboundHandler {
typealias OutboundIn = Any

var closes = 0

func close(context: ChannelHandlerContext, mode: CloseMode, promise: EventLoopPromise<Void>?) {
self.closes += 1
if self.closes == 1 {
promise?.fail(DummyError())
} else {
context.close(mode: mode, promise: promise)
}
}
}

let channel = try! assertNoThrowWithValue(ServerBootstrap(group: group).serverChannelInitializer { channel in
channel.pipeline.addHandler(MakeFirstCloseFailAndDontActuallyCloseHandler(), position: .first)
}.bind(host: "localhost", port: 0).wait())
defer {
XCTAssertNoThrow(try channel.close().wait())
}

XCTAssertNoThrow(try channel.pipeline.addHandler(counter).wait())

XCTAssertNoThrow(try channel.eventLoop.submit {
// this will trigger a close (which will fail and also not actually close)
channel.pipeline.fireUserInboundEventTriggered(ChannelShouldQuiesceEvent())
}.wait())
XCTAssertEqual(["userInboundEventTriggered", "close", "errorCaught"], counter.allTriggeredEvents())
XCTAssertEqual(1, counter.errorCaughtCalls)
}
}

fileprivate final class FailRegistrationAndDelayCloseHandler: ChannelOutboundHandler {

0 comments on commit ca92786

Please sign in to comment.
You can’t perform that action at this time.