Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core: add description to Status.UNKNOWN in ServerImpl's #internalClose #10643

Merged
merged 5 commits into from
Nov 6, 2023
Merged

core: add description to Status.UNKNOWN in ServerImpl's #internalClose #10643

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21a742a
core: add description to Status.UNKNOWN in ServerImpl's #internalClose
sergiitk Nov 2, 2023
272c3f1
Fix the tests
sergiitk Nov 2, 2023
f7f2a32
Remove "Internal" from "Internal Application Error"
sergiitk Nov 2, 2023
6303c75
Update message in the test as well
sergiitk Nov 2, 2023
87e0179
Undo `@ task` extra context info per review
sergiitk Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions core/src/main/java/io/grpc/internal/ServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,9 +808,10 @@ void setListener(ServerStreamListener listener) {
/**
* Like {@link ServerCall#close(Status, Metadata)}, but thread-safe for internal use.
*/
private void internalClose(Throwable t) {
private void internalClose(Throwable t, String task) {
// TODO(ejona86): this is not thread-safe :)
stream.close(Status.UNKNOWN.withCause(t), new Metadata());
String description = "Application Error @ task " + task;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considered @ context instead of @ task. Let me know if you like one or another better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use "Application Error while processing " + message/half close/onReady. The trace task name is too internally oriented for a good user message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ejona86 what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stream.close(Status.UNKNOWN.withDescription(description).withCause(t), new Metadata());
}

@Override
Expand All @@ -826,13 +827,13 @@ final class MessagesAvailable extends ContextRunnable {

@Override
public void runInContext() {
try (TaskCloseable ignore =
PerfMark.traceTask("ServerCallListener(app).messagesAvailable")) {
String taskName = "ServerCallListener(app).messagesAvailable";
try (TaskCloseable ignore = PerfMark.traceTask(taskName)) {
PerfMark.attachTag(tag);
PerfMark.linkIn(link);
getListener().messagesAvailable(producer);
} catch (Throwable t) {
internalClose(t);
internalClose(t, taskName);
throw t;
}
}
Expand All @@ -854,12 +855,13 @@ final class HalfClosed extends ContextRunnable {

@Override
public void runInContext() {
try (TaskCloseable ignore = PerfMark.traceTask("ServerCallListener(app).halfClosed")) {
String taskName = "ServerCallListener(app).halfClosed";
try (TaskCloseable ignore = PerfMark.traceTask(taskName)) {
PerfMark.attachTag(tag);
PerfMark.linkIn(link);
getListener().halfClosed();
} catch (Throwable t) {
internalClose(t);
internalClose(t, taskName);
throw t;
}
}
Expand Down Expand Up @@ -927,12 +929,13 @@ final class OnReady extends ContextRunnable {

@Override
public void runInContext() {
try (TaskCloseable ignore = PerfMark.traceTask("ServerCallListener(app).onReady")) {
String taskName = "ServerCallListener(app).onReady";
try (TaskCloseable ignore = PerfMark.traceTask(taskName)) {
PerfMark.attachTag(tag);
PerfMark.linkIn(link);
getListener().onReady();
} catch (Throwable t) {
internalClose(t);
internalClose(t, taskName);
throw t;
}
}
Expand Down
7 changes: 6 additions & 1 deletion interop-testing/src/test/java/io/grpc/testing/integration/MoreInProcessTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,12 @@ public void onCompleted() {
.onNext(StreamingInputCallRequest.getDefaultInstance());

assertTrue(finishLatch.await(900, TimeUnit.MILLISECONDS));
assertEquals(Status.UNKNOWN, Status.fromThrowable(throwableRef.get()));
Status actualStatus = Status.fromThrowable(throwableRef.get());
Status expectedStatus = Status.UNKNOWN.withDescription(
"Internal Application Error @ task ServerCallListener(app).messagesAvailable");
assertEquals(expectedStatus.getCode(), actualStatus.getCode());
assertEquals(expectedStatus.getDescription(), actualStatus.getDescription());
larry-safran marked this conversation as resolved.
Show resolved Hide resolved
assertNull(actualStatus.getCause());
assertNull(responseRef.get());
}
}
Loading