Remove agressive precondition in zero-length-writes. (#108)

Motivation:

The zero-length writes handler will crash if it receives channelInactive
before channelActive. Sadly, that's totally possible if a channel is
closed from the connect promise.

Modifications:

- Tone back the preconditions a bit.
- Add a test that triggers this path.

Result:

Channel handler behaves better on early close
This commit is contained in:
Cory Benfield 2020-11-24 13:21:50 +00:00 committed by GitHub
parent 18c17b9ae0
commit 5a352330c0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 29 additions and 6 deletions

View File

@ -88,9 +88,16 @@ public final class NIOFilterEmptyWritesHandler: ChannelDuplexHandler {
// Connection state management
extension NIOFilterEmptyWritesHandler {
public func channelActive(context: ChannelHandlerContext) {
assert(self.state == .notActiveYet)
self.state = .open
context.fireChannelActive()
switch self.state {
case .notActiveYet:
self.state = .open
context.fireChannelActive()
case .closedFromLocal:
// Closed before we activated, not a problem.
assert(self.prefixEmptyWritePromise == nil)
case .open, .closedFromRemote, .error:
preconditionFailure()
}
}
public func channelInactive(context: ChannelHandlerContext) {
@ -117,15 +124,17 @@ extension NIOFilterEmptyWritesHandler {
switch (mode, self.state) {
case (.all, .open),
(.output, .open):
(.output, .open),
// We allow closure in .notActiveYet because it is possible to close before the channelActive fires.
(.all, .notActiveYet),
(.output, .notActiveYet):
self.state = .closedFromLocal
save?.fail(ChannelError.outputClosed)
case (.all, .closedFromLocal),
(.output, .closedFromLocal),
(.all, .closedFromRemote),
(.output, .closedFromRemote),
(.all, .notActiveYet),
(.output, .notActiveYet),
(.all, .error),
(.output, .error):
assert(save == nil)

View File

@ -260,5 +260,19 @@ class NIOFilterEmptyWritesHandlerTests: XCTestCase {
)
thenEmptyWritePromise = nil
}
func testCloseOnConnect() {
/// This test reproduces https://github.com/grpc/grpc-swift/issues/1014. It does so by testing
/// that a precondition _does not fire_. This test makes no assertions, it just shouldn't crash.
let channel = EmbeddedChannel(handler: NIOFilterEmptyWritesHandler())
let promise = channel.eventLoop.makePromise(of: Void.self)
let future: EventLoopFuture<Void> = promise.futureResult.flatMap {
XCTAssertTrue(channel.isActive)
return channel.close()
}
channel.connect(to: try! .init(ipAddress: "1.1.1.1", port: 1), promise: promise)
XCTAssertNoThrow(try future.wait())
XCTAssertFalse(channel.isActive)
}
}
#endif