hashgraph / pbj

A performance optimized Google Protocol Buffers code generator, parser, and Gradle module.
Apache License 2.0
24 stars 6 forks source link

Helidon plugin serverStreaming gRPC method immediately closes connection #300

Closed mattp-swirldslabs closed 1 month ago

mattp-swirldslabs commented 1 month ago

Description

While integrating the Helidon plugin into Block Node, I attempted to open a streaming server connection but it was immediately closed by PbjProtocolHandler. The same gRPC method invocation against grpc.io will hold the connection open waiting for data to stream back.

Steps to reproduce

  1. Setup a server streaming method which uses an Executor service to use the Flow.Subscriber to send back a stream of responses asynchronously.
  2. Call the method using grpcurl. With grpc.io, the connection will be held open while the responses are received. However, with the Helidon plugin, it PbjProtocolHandler immediately closes the connection.

Additional context

No response

Hedera network

other

Version

v0.9.9

Operating system

macOS

litt3 commented 1 month ago

Update so far:

Deleting incoming.onComplete(); (line 384) in PbjProtocolHandler.java causes the connection to be held open waiting for data to stream back.

The javadoc (in java.util.concurrent.Flow) for this call reads Method invoked when it is known that no additional Subscriber method invocations will occur for a Subscription that is not already terminated by error, after which no other Subscriber methods are invoked by the Subscription. If this method throws an exception, resulting behavior is undefined.

There are 2 potential answers to this problem:

  1. This call ought to be called somewhere, but not here. I am investigating where else this call might belong.
  2. The test procedure is incorrect, and is sending a premature END_OF_STREAM http2 flag. The helidon implementation correctly identifies this flag, and behaves correctly, and the previous implementation was incorrect
litt3 commented 1 month ago

I confirmed via wireshark that running echo "{\"start_block_number\": 1}" | grpcurl -vv -plaintext -proto ./block_service.proto -d @ localhost:8080 com.hedera.hapi.block.BlockStreamService/subscribeBlockStream is sending an End Stream flag

HyperText Transfer Protocol 2
    Stream: DATA, Stream ID: 1, Length 7
        Length: 7
        Type: DATA (0)
        Flags: 0x01, End Stream
            0000 .00. = Unused: 0x00
            .... 0... = Padded: False
            .... ...1 = End Stream: True
        0... .... .... .... .... .... .... .... = Reserved: 0x0
        .000 0000 0000 0000 0000 0000 0000 0001 = Stream Identifier: 1
        [Pad Length: 0]
        DATA payload (7 bytes)
        [Connection window size (before): 1048576]
        [Connection window size (after): 1048569]
        [Stream window size (before): 1048576]
        [Stream window size (after): 1048569]

According to the HTTP/2 spec, the End Stream flag should result in the stream being closed:

END_STREAM (0x01): When set, the END_STREAM flag indicates that this frame is the last that the endpoint will send for the identified stream. Setting this flag causes the stream to enter one of [the "half-closed" states or the "closed" state]

Therefore, I think the correct conclusion is that the previous implementation is out-of-spec when it doesn't close the streaming connection, having received this flag.

I think the correct solution is to devise an alternate testing mechanism, that doesn't send a premature end stream flag. I have noticed that running grpcurl -vv -plaintext -proto ./block_service.proto -d @ localhost:8080 com.hedera.hapi.block.BlockStreamService/subscribeBlockStream without any echo input doesn't cause the stream to close, so this may be an option.

litt3 commented 1 month ago

Done