Repository: apple/swift-nio
File: Tests/NIOCoreTests/SingleStepByteToMessageDecoderTest.swift (Swift)

Code before:
    /// Tests re-entrancy by having a nested decoding operation empty the buffer and exit part way
    /// through the outer processing step
    func testErrorDuringNestedDecoding() {
        class ThrowingOnLastDecoder: NIOSingleStepByteToMessageDecoder {
            /// `ByteBuffer` is the expected type passed in.
            public typealias InboundIn = ByteBuffer
            /// `ByteBuffer`s will be passed to the next stage.
            public typealias InboundOut = ByteBuffer

            public init() { }

            struct DecodeLastError: Error {}

            func decodeLast(buffer: inout NIOCore.ByteBuffer, seenEOF: Bool) throws -> NIOCore.ByteBuffer? {
                throw DecodeLastError()
            }

            func decode(buffer: inout NIOCore.ByteBuffer) throws -> NIOCore.ByteBuffer? {
                ByteBuffer()
            }
        }

        let decoder = ThrowingOnLastDecoder()
        let b2mp = NIOSingleStepByteToMessageProcessor(decoder)
        XCTAssertNoThrow(try b2mp.process(buffer: ByteBuffer(string: "1\n\n2\n3\n")) { line in
            XCTAssertThrowsError(try b2mp.finishProcessing(seenEOF: true) { _ in }) { error in
                XCTAssertNotNil(error as? ThrowingOnLastDecoder.DecodeLastError)
            }
        })
    }
}


Review comment (bug):
This test is testing your fix, but unless I misunderstood your comment before the test, I think it's not happening in the way you expected.

`XCTAssertThrowsError` isn't actually throwing, so the closure passed to `process(buffer:messageReceiver:)` won't actually throw when executed. 

This isn't all bad though, because that means that the buffer will be nil after the given closure, which will test your fix as the method will continue without erroring when force-unwrapping the buffer. However, that's not what you were set to test here, since we're not exiting part way.

I think it would make sense to keep this test (if we don't already have one that tests the `messageReceiver` being throwing), but removing the `XCTAssertThrowsError` so that `finishProcessing` actually throws, and we can assert that `process` throws when the inner closure fails. 

However, I would definitely have a separate test to test the actual fix here: to do that you can get rid of the error being thrown on `decodeLast`: we just need to call `finishProcessing` but make sure it doesn't throw to trigger the re-entrancy bug.

Code after:
    /// Tests re-entrancy by having a nested decoding operation empty the buffer and exit part way
    /// through the outer processing step
    func testErrorDuringNestedDecoding() {
        class ThrowingOnLastDecoder: NIOSingleStepByteToMessageDecoder {
            /// `ByteBuffer` is the expected type passed in.
            public typealias InboundIn = ByteBuffer
            /// `ByteBuffer`s will be passed to the next stage.
            public typealias InboundOut = ByteBuffer

            public init() {}

            struct DecodeLastError: Error {}

            func decodeLast(buffer: inout NIOCore.ByteBuffer, seenEOF: Bool) throws -> NIOCore.ByteBuffer? {
                buffer = ByteBuffer()  // to allow the decode loop to exit
                throw DecodeLastError()
            }

            func decode(buffer: inout NIOCore.ByteBuffer) throws -> NIOCore.ByteBuffer? {
                ByteBuffer()
            }
        }

        let decoder = ThrowingOnLastDecoder()
        let b2mp = NIOSingleStepByteToMessageProcessor(decoder)
        var errorObserved = false
        XCTAssertNoThrow(
            try b2mp.process(buffer: ByteBuffer(string: "1\n\n2\n3\n")) { line in
                // We will throw an error to exit the decoding within the nested process call prematurely.
                // Unless this is carefully handled we can be left in an inconsistent state which the outer call will encounter
                do {
                    try b2mp.finishProcessing(seenEOF: true) { _ in }
                } catch _ as ThrowingOnLastDecoder.DecodeLastError {
                    errorObserved = true
                }
            }
        )
        XCTAssertTrue(errorObserved)
    }
}