EOF behaviour in bi-di streams

[tjungblu]

Hey!

I'm coming from etcd-io/etcd#13909 where I need some help to understand how the bidirectional streaming is supposed to work. etcd uses the grpc-gateway in front of the grpc-proxy which in turn runs against the etcd server. etcd has a feature called "watches" where you get a stream of all changes of the database.

For some reason, the gateway was implemented with a stream of WatchRequests, allowing you to create, cancel watch requests from the client stream. The server will then forward the requests and just proxy all results back.

This feature is fairly old (>6 years) and seems that it used to work using curl at some point as documented here:
https://etcd.io/docs/v3.5/dev-guide/api_grpc_gateway/#watch-keys

Since it's a bit complicated to set etcd up with the proxy and such, I replicated the issue as minimally as possible below using an integration test, which luckily shows the same error:

master...tjungblu:grpc-gateway:repro

TL;DR; the bidi stream results in EOF errors:

    --- FAIL: TestBidiStream/bidi_stream_case (0.00s)
        integration_test.go:1814: resp.StatusCode = 500; want 200
        integration_test.go:1818: response = [{"error":{"code":2,"message":"EOF","details":[]}}]; want [{"result":{"data":"some-text"}} {"result":{"data":"some-text"}}]

🐛 Bug Report

The sequence of events is the following:

1. request comes in, containing the initial request
2. request gets decoded, no error and passed along
3. body is now empty, EOF is returned by the decoder and the loop stops
4. the loop then returns either context canceled due to the close or the EOF error itself (which is also a race condition problem here)

To Reproduce

Checkout the branch in my fork:
master...tjungblu:grpc-gateway:repro

then just run make test to observe the test failure.

Expected behavior

That test should pass and I should receive a 200 with the stream of elements until I close the connection of the post request. Which kinda requires a solution for #2434 as well...

Actual Behavior

Test fails :) Please let me know if we're using it wrong here, I was just picking this up from an issue report.

Your Environment

Fedora 36, go version go1.18.2 linux/amd64

[johanbrandhorst]

Thanks for your issue report! I can't say the grpc-gateway has ever intentionally supported bi-directional streaming (see https://github.com/grpc-ecosystem/grpc-gateway#no-plan-to-support), if etc-d somehow relies on this I would recommend that you migrate away from it, or consider wrapping the gateway in the grpc-websocket-proxy which makes a best effort to support bidi streams over websockets: https://github.com/tmc/grpc-websocket-proxy.

That said, I'd be happy to try and work out the best thing we can do here, the failing test is a great start. Would you be interested in trying to fix this yourself? Almost 100% of our changes these days are from contributors like yourself and I'd be happy to discuss designs and review PRs but don't have time to make fixes myself.

[tjungblu]

I would recommend that you migrate away from it,

That certainly sounds sensible, I'm mostly wondering if it worked at some point and something underneath changed. For example the decoder behavior or the body stream.

That said, I'd be happy to try and work out the best thing we can do here, the failing test is a great start. Would you be interested in trying to fix this yourself? Almost 100% of our changes these days are from contributors like yourself and I'd be happy to discuss designs and review PRs but don't have time to make fixes myself.

Sure, I can take a look and see if this can be fixed somehow, I can probably get to it by Friday or later next week.

[tjungblu]

@johanbrandhorst so the fix to get the test green is easy:
https://github.com/grpc-ecosystem/grpc-gateway/pull/2972/files#diff-9648d8792ce733c8bbce0d613a620fffdb73cff98a91763507a1984eb520d5feL440-L444

https://github.com/grpc-ecosystem/grpc-gateway/pull/2972/files#

BUT, huge caveat, this busy loops on the request body stream. The question is, how can we make this smarter so the decode only runs once sufficient bytes are available? Do you have a good idea?

There's nothing really fitting in the sdk, so we would need to hand-roll some "notification" io.reader I think...

[johanbrandhorst]

Hm, it looks to me like it should in theory wait for a full new message to be read by dec.Decode, so it doesn't sound like a busy loop, although have you tested it and found it to be a busy loop? I really don't even want to think about going down the path of some sort of external notification system for this. If req.Body is streaming and dec.Decode blocks until it's read a full message, this looks OK? Although continuing on io.EOF seems like it might cause the busy loop you're talking about, is that what you mean?

[tjungblu]

if req.Body is streaming and dec.Decode blocks until it's read a full message, this looks OK?

I assume this used to be the behavior for this a while back.

Although continuing on io.EOF seems like it might cause the busy loop you're talking about, is that what you mean?

yep, that's it. We can maybe continue the conversation on the PR, if that's a bit easier to talk about the specific lines.

[tjungblu]

Let's close this out here, I think we won't be able to solve this bidirectional streaming issue with a busy looping efficiently.