Comments (14)
@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):
-
create a test that can reliably reproduce the scenario where
SendMsg
returnsio.EOF
(server closes connection before client actually sends?) and the client doesn't get the expected actual error -
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 hasClientStreams
set tofalse
-
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 returnEOF
to indicate end of stream -
the upper level
RecvMsg
gets theEOF
returned and callsfinish
with it. Infinish
we convertEOF
tonil
, but the conversion isn't likely to be reflected inRecv
because the original error is passed by value instead of by reference. Therefore it's possible forRecvMsg
to returnEOF
.
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 ofSendMsg()
. It's a matter of moving theif
.
from grpc-go.
@dfawley Whoops, turned out initially I had some misunderstanding on the issue 😅 But I think I get it now (thanks to @ash2k 's response):
- emulating the unary RPC client's
SendMsg
&RecvMsg
sequence on a streaming RPC client doesn't quite work (repro), because an additional check forEOF
fromSendMsg
is needed - the
SendMsg
&RecvMsg
sequence does work for unary RPC (i.e.invoke
) though, because of the behaviorSendMsg
has on non-streaming RPC type - we might want to adjust documentation/implementation of
SendMsg
per suggested by @ash2k to avoid potential confusion (in our case it makes user suspect thatinvoke
doesn't handle return value ofSendMsg
properly)
Sounds like documentation update is an easier option, but let me know if there's more to considerate 😃
from grpc-go.
I'm leaning towards getting rid of the special handling of non-streaming RPCs in sendMsg and handling the io.EOF in invoke. This would unify the behaviour for both kinds of RPCs and make the function easier to reason about.
The special handling in sendMsg has been there for for quite some time and the comment was changed 6 years ago in #1854
So I'm concerned about accidentally changing any user facing behaviour. Let me discuss with the team and share an update.
from grpc-go.
Assigning to @dfawley to comment on the preferred fix. Since he's presently on vacation, expect a reply by end of the week.
from grpc-go.
Returning io.EOF
here would break existing code quite badly - our generated code relies upon SendMsg
not returning an error on success. So we should update the docs to say something like: for ClientStreams=false
streams, nil
is returned unconditionally.
from grpc-go.
Related Issues (20)
- Linter rule for using context.Background() without a timeout in tests HOT 4
- gRPC is incompatible with tls.Listener HOT 2
- Closing connection takes up to 15 minutes. HOT 5
- Feature Request: expose handleRawConn or add ServeConn HOT 26
- Flaky test: TimerAndWatchStateOnErrorCallback HOT 4
- xds: bootstrap config is not emitted to logs in a human readable way
- Strongly-type request inside a Stream Server Interceptor HOT 2
- Proxy connection buffer necessary? HOT 1
- Why does grpc.NewClient silently ignore DialOptions? HOT 2
- Make transport.SetConnection public? HOT 4
- what's the default max data size
- If a priority contains multiple localities with pick_first, load is reported incorrectly HOT 4
- NewClient doesn't work with WithContextDialer HOT 2
- xds: make the bootstrap configuration a singleton
- protoc gen adds comment, does not follow godoc. HOT 5
- Flaky test: Test/AggregateCluster_BadDNS_GoodEDS HOT 3
- Setting HTTPS proxy without setting HTTPS_PROXY env variable HOT 2
- channelz has a startup sequencing bug HOT 2
- ring_hash stuck in TransientFailure despite having available endpoints HOT 1
- Flaky Test/AuthorityRevive in google.golang.org/grpc/xds/internal/xdsclient/tests HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from grpc-go.