Status propagation changes #16418
Merged
Status propagation changes #16418
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
ncteisen
approved these changes
Aug 24, 2018
src/core/lib/surface/call.cc
Outdated
| grpc_error_get_status(error, call->send_deadline, | ||
| call->final_op.client.status, &slice, nullptr, | ||
| call->final_op.client.error_string); | ||
| *call->final_op.client.status_details = grpc_slice_ref_internal(slice); |
There was a problem hiding this comment.
That could still be done without creating the extra slice on the stack.
grpc_error_get_status(error, call->send_deadline,
call->final_op.client.status, call->final_op.client.status_details,
nullptr, call->final_op.client.error_string);
// explicitly take a ref
grpc_slice_ref_internal(call->final_op.client.status_details);
|
|
|
|
|
|
|
|
|
|
|
|
|
Known issues : #16471 |
|
Thanks for reviewing! |
This was referenced Sep 4, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of Ken's PR #13368
Rough explanation of why this new logic is roughly equivalent to the old logic:
The old logic would search through a bunch of errors attached to the recv_trailing_metadata batch, looking for an error with an explicit non-ok status, and sort these errors according to their source.
One of the errors was synthesized from the receive_trailing_metadata op. This status would always be explicit, and had the second highest priority STATUS_FROM_WIRE. In the case of the chttp2 transport, whenever one of the other batch errors occurred, it would cause a cancellation and a fake receive_trailing_metadata status was created, and this would be the resulting status code. Other transports would end up failing the receive_trailing_metadata batch with this error.
New Approach:
Surface: The call status will always be determined by the recv_trailing_metadata_ready callback. If the callback completes with an error, that error is used to determine the status code. If the callback completes without an error, the status code is determined from the grpc-status metdata value.
Any ops that complete with an error will cause a cancellation to occur on the entire call.
Filter Stack: If a filter stack injects an error on the way down (on a send op), the entire batch should be failed with that error. This will cause any recv callbacks to be called with that error.
If a filter stack injects an error on the way up (recv{metadata/msg/trailing_metadata} callback), that error must also be injected into the trailing_metadata callback. If there is already an error in the trailing_metadata callback, that error should be added as a child.
Transport: If a transport receives a cancellation op, the first cancellation error must be passed to recv_trailing_metadata_ready (if it has not been queued already).