Fix wrong exceptions being thrown on send failure. #8084
Conversation
| origTcs.SetException(new InvalidOperationException("Send failed")); | ||
| if (!delayCompletion) | ||
| { | ||
| origTcs.SetException(IsClient ? GetRpcExceptionClientOnly() : new IOException("Error sending from server.")); |
There was a problem hiding this comment.
So we only fill in the exception for the SendMessageTask here if the final call completion handler has already been ran through, meaning the GetRpcExceptionClientOnly will always be able to fill in with a non-null FinalStatus? Maybe use a sort of invariant if so? Got caught here for a little bit with the delayCompletion flag
There was a problem hiding this comment.
Yes, you understand that correctly, but you are right that the code was a bit cryptic. I tried to improved that by adding a precondition and some comments.
jtattermusch
force-pushed
the
throw_rpcexception_on_failure
branch
from
1638aca to
a610e32
Compare
|
Addressed the comments, PTAL. |
| // The write will wait for call to finish to receive the status code. | ||
| Assert.IsFalse(writeTask.IsCompleted); | ||
|
|
||
| fakeCall.UnaryResponseClientHandler(true, |
There was a problem hiding this comment.
the StatusCode.Internal being the made up status core will create?
| TaskCompletionSource<object> origTcs = null; | ||
| lock (myLock) | ||
| { | ||
| origTcs = streamingWriteTcs; | ||
| streamingWriteTcs = null; | ||
|
|
||
| if (!success && !finished && IsClient) | ||
| { | ||
| // We should be setting this only once per call, following writes will be short circuited. |
There was a problem hiding this comment.
They get short circuited later only because the next call can't start until the current one, which waits on the final status, is complete?
There was a problem hiding this comment.
Actually, good question. There was a problem with the implementation - because streamingWriteTcs was reset here, if there was a delayed write completion, one could have started another write even though the failed write hasn't finished yet from user's perspective (and that also meant that the statement in the comment was incorrect).
I changed the implementation to use a boolean flag isDelayedStreamingWriteCompletion and I'm leaving streamingWriteTcs set until the call actually finishes. Now everything should be alright hopefully. I also added a test that was exposing the issue with the implementation (it is passing now).
PTAL. Hopefully no more fixes will be needed.
|
LGTM for changes in last two commits |
Tentative fix for #7223.