HTTP2 go away at the protocol level lets the request hang

[dnadoba]

We currently ignore the error code of the go away. I think if it is a protocol error we will never receive any further HTTP2Frame on the connection or any stream and need manually cancel all active streams. Maybe this is better handled at the nio http2 level but I'm not sure yet.

Reproducer:

    func testGoAwayFromServerDoesntHangRequest() throws {
        //try XCTSkipIf(true, "this currently hangs")
        let bin = HTTPBin(.http2())
        defer { XCTAssertNoThrow(try bin.shutdown()) }
        
        let loggerFactor = StreamLogHandler.standardOutput(label:)
        var bgLogger = Logger(label: "BG", factory: loggerFactor)
        bgLogger.logLevel = .trace
        
        let client = HTTPClient(
            eventLoopGroupProvider: .createNew,
            configuration: .init(certificateVerification: .none),
            backgroundActivityLogger: bgLogger
        )
        
        defer { XCTAssertNoThrow(try client.syncShutdown()) }
        
        var request = try HTTPClient.Request(url: bin.baseURL, method: .POST)
        // add ~64 KB header
        let headerValue = String(repeating: "0", count: 1024)
        for headerID in 0..<64 {
            request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue)
        }

        // non empty body is important to trigger this bug as we otherwise finish the request in a single flush
        request.body = .byteBuffer(ByteBuffer(bytes: [0]))

        var rqLogger = Logger(label: "RQ", factory: loggerFactor)
        rqLogger.logLevel = .trace
        
        XCTAssertThrows(try client.execute(request: request, deadline: .distantFuture, logger: rqLogger).wait())
    }

[Lukasa]

Whether streams will continue is dependent on the "last stream ID" in a GOAWAY.

[dnadoba]

So I see this go away frame received on the client side:

HTTP2Frame(streamID: HTTP2StreamID(0), payload: .goAway(lastStreamID: HTTP2StreamID(2147483647), errorCode: HTTP2ErrorCode<0x1 ProtocolError>, opaqueData: nil))

I think the relevant RFC section is this:

A server that is attempting to gracefully shut down a connection SHOULD send an initial GOAWAY frame with the last stream identifier set to 231-1 and a NO_ERROR code. This signals to the client that a shutdown is imminent and that initiating further requests is prohibited. After allowing time for any in-flight stream creation (at least one round-trip time), the server MAY send another GOAWAY frame with an updated last stream identifier. This ensures that a connection can be cleanly shut down without losing requests.¶

However, the error code is protocol error and not no error.

[Lukasa]

Yeah, this isn't graceful shutdown, it's definitely an error. But PROTOCOL_ERROR doesn't necessarily imply that the connection is lost, merely that a protocol error was committed. If the server wants us to stop talking, it needs to be more emphatic than this.