core: add description to Status.UNKNOWN in ServerImpl's #internalClose #10643
Conversation
This is currently the only place where we return Status.UNKNOWN with no description, which makes is harder to debug and differentiate from statuses originated from non-grpc sources. This PR enriches ServerImpl's #internalClose `Status.UNKNOWN` with description `Internal Application Error` and appends context information for the origin of the error; one of: - ServerCallListener(app).messagesAvailable - ServerCallListener(app).halfClosed - ServerCallListener(app).onReady
| // TODO(ejona86): this is not thread-safe :) | ||
| stream.close(Status.UNKNOWN.withCause(t), new Metadata()); | ||
| String description = "Application Error @ task " + task; |
There was a problem hiding this comment.
Considered @ context instead of @ task. Let me know if you like one or another better.
There was a problem hiding this comment.
I would use "Application Error while processing " + message/half close/onReady. The trace task name is too internally oriented for a good user message.
There was a problem hiding this comment.
@ejona86 what do you think?
There was a problem hiding this comment.
I'd suggest "Application error", "Application error processing RPC", or such. We have logged for the service authors to investigate what is wrong, we try not to tell the client too much because it could be an attacker, and the strings here are probably misleading. For that last point, the problem is the stubs morph things; most RPCs that throw will be within "half close" because they are unary and that's the point we call the application. But few know that. Including that information seems likely to just steer someone in the wrong direction.
interop-testing/src/test/java/io/grpc/testing/integration/MoreInProcessTest.java
Show resolved
Hide resolved
| // TODO(ejona86): this is not thread-safe :) | ||
| stream.close(Status.UNKNOWN.withCause(t), new Metadata()); | ||
| String description = "Application Error @ task " + task; |
There was a problem hiding this comment.
I would use "Application Error while processing " + message/half close/onReady. The trace task name is too internally oriented for a good user message.
…rnalClose (grpc#10643)" This reverts commit 8b4b14a.
This is currently the only place where we return Status.UNKNOWN with no description, which makes is harder to debug and differentiate from statuses originated from non-grpc sources.
This PR enriches ServerImpl's #internalClose
Status.UNKNOWNwith descriptionApplication error processing RPC.