B2MD: don't go to final state twice (#948)

Motivation:

B2MD (in debug mode) has some checking that we never move out of a final
state in the state machine. Both .done and .error are final states.
Previously however, the code set .done in a defer block which meant that
even if the decodeLoop throws (which should bring it to .error), it goes
via .done first. That's an error because .done is already a final state
so we shouldn't move to .error after that.

Modifications:

Only move to .done if there was no error. If there was an error, we
should go straight to .error, not via .done.

Result:

- more correct B2MD
- fewer crashes (in debug mode)

Sources/NIO/Codec.swift

@@ -523,15 +523,13 @@ extension ByteToMessageHandler: ChannelInboundHandler {
                     () // fair, all done already
                 case .leftoversNeedProcessing:
                     // seems like we received a `channelInactive` or `handlerRemoved` whilst we were processing a read
-                    defer {
-                        self.state = .done
-                    }
                     switch try self.decodeLoop(context: context, decodeMode: .last) {
                     case .didProcess:
                         () // expected and cool
                     case .cannotProcessReentrantly:
                         preconditionFailure("bug in NIO: non-reentrant decode loop couldn't run \(self), \(self.state)")
                     }
+                    self.state = .done
                 }
             case .cannotProcessReentrantly:
                 // fine, will be done later

Tests/NIOTests/CodecTest+XCTest.swift

@@ -54,6 +54,7 @@ extension ByteToMessageDecoderTest {
                 ("testDecodeLoopStopsOnChannelInactive", testDecodeLoopStopsOnChannelInactive),
                 ("testDecodeLoopStopsOnInboundHalfClosure", testDecodeLoopStopsOnInboundHalfClosure),
                 ("testWeForwardReadEOFAndChannelInactive", testWeForwardReadEOFAndChannelInactive),
+                ("testErrorInDecodeLastWhenCloseIsReceivedReentrantlyInDecode", testErrorInDecodeLastWhenCloseIsReceivedReentrantlyInDecode),
            ]
    }
 }

Tests/NIOTests/CodecTest.swift

@@ -1304,6 +1304,31 @@ public final class ByteToMessageDecoderTest: XCTestCase {
         XCTAssertEqual(1, checker.channelInactiveEvents)
         XCTAssertEqual(1, checker.readEOFEvents)
     }
+
+    func testErrorInDecodeLastWhenCloseIsReceivedReentrantlyInDecode() {
+        struct DummyError: Error {}
+        struct Decoder: ByteToMessageDecoder {
+            typealias InboundOut = Never
+
+            func decode(context: ChannelHandlerContext, buffer: inout ByteBuffer) throws -> DecodingState {
+                // simulate a re-entrant trigger of reading EOF
+                context.channel.pipeline.fireUserInboundEventTriggered(ChannelEvent.inputClosed)
+                return .needMoreData
+            }
+
+            func decodeLast(context: ChannelHandlerContext, buffer: inout ByteBuffer, seenEOF: Bool) throws -> DecodingState {
+                XCTAssertEqual("X", buffer.readString(length: buffer.readableBytes))
+                throw DummyError()
+            }
+        }
+
+        let channel = EmbeddedChannel(handler: ByteToMessageHandler(Decoder()))
+        var buffer = channel.allocator.buffer(capacity: 1)
+        buffer.writeString("X")
+        XCTAssertThrowsError(try channel.writeInbound(buffer)) { error in
+            XCTAssertTrue(error is DummyError)
+        }
+    }
 }
 
 public final class MessageToByteEncoderTest: XCTestCase {