Return the actual error from unary RPCs when `SendMsg()` returns `io.EOF` about grpc-go HOT 10 OPEN

@Kailun2047 Sure, that sounds good to me, as long as you think you can work on it pretty soon.

For the repro, I wonder if we can reproduce it by making a large request, and configure the server with a very small initial window size. That way the client won't be able to send the full request as long as the server handler treats the method as streaming. That should make it 100% reproducible, and be fairly simple to do.

from grpc-go.

Yes, I think this is just an oversight in our code here. If SendMsg returns io.EOF, we should call RecvMsg and use the error there. I believe it's possible for the RPC to end successfully even if the message can't be sent, so if RecvMsg returns nil, then the RPC should end with a success. There should definitely be a test for this. The server handler needs to be a streaming RPC handler that immediately ends the RPC (both success & failure should be tested). I'm not sure if we can inject something into the connection to make sends from the client to the server slower in order to stimulate the race more reliably, but that's one possible option.

from grpc-go.

To be clear, I don't have bandwidth to work on the fix myself.

from grpc-go.

No problem! This is rare but probably important enough that we should prioritize this internally. Also the fix is probably quite easy.

from grpc-go.

@dfawley Hi, is it possible for me pick this one up? Looks like we need to get the following done (please correct me if I'm wrong):

1. create a test that can reliably reproduce the scenario where SendMsg returns io.EOF (server closes connection before client actually sends?) and the client doesn't get the expected actual error

2. implement the proposed solution and make sure it make the test pass

from grpc-go.

@dfawley Got it. Will try to delve in and make an update here within a day or two.

from grpc-go.

@dfawley Hi, I tried creating a test per earlier discussion to reproduce but had no luck 😅 On StubServer, SendMsg always succeeds even when the request payload size is increased to the max allowable (4194304).

But upon some initial inspection, looks like the EOF reported might not originate from SendMsg. Concretely, for invocation of an unary RPC,

• client stream is created with unaryStreamDesc, which has ClientStreams set to false

• when sending message with the client stream, the stream will not return EOF for non-client-streaming RPCs

Instead, it appears to me that the source of the EOF might be a different issue in RecvMsg. Concretely,

• recvMsg might return EOF to indicate end of stream

• the upper level RecvMsg gets the EOF returned and calls finish with it. In finish we convert EOF to nil, but the conversion isn't likely to be reflected in Recv because the original error is passed by value instead of by reference. Therefore it's possible for RecvMsg to return EOF.

I'm definitely willing to put more investigation into this, but let me know if you are concerned about the timing and need to prioritize internally 😄

from grpc-go.

Oh interesting. I thought it was something simpler, but you're right - some of the streaming code actually checks the type of the RPC and adjusts its behavior accordingly. If you're able to get to the bottom of this, we would definitely appreciate it. No worries on the timing at all - I don't think it's as urgent if it's a pretty rare occurrence.

from grpc-go.

@ash2k Hi - since we now might have a bigger space to search for the potential source of the EOF, is it possible to share more info about the test mentioned in OP? Specifically, are there any ServerOption/DialOption/CallOption being used there? Any other info will also be appreciated. Thanks!

The existing gRPC tests around Invoke (in which server also return error immediately) don't seem to help reproduce so I figure there could be something missing.

from grpc-go.

Without going into details, my code emulates a unary RPC over a streaming RPC. That's probably why you cannot reproduce the issue.

when sending message with the client stream, the stream will not return EOF for non-client-streaming RPCs

This means code in invoke() relies on an undocumented behavior that EOF is not returned from SendMsg() if it's a unary RPC. I see two options:

• This behavior should be documented as part of the SendMsg() doc.
• Special behavior that you linked to can be removed and code in invoke() should handle the general documented behavior of SendMsg(). It's a matter of moving the if.

from grpc-go.