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

StatusRuntimeException without stacktrace - Android compatibility #11072

Merged
merged 2 commits into from
Dec 22, 2024
Merged

StatusRuntimeException without stacktrace - Android compatibility #11072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82e2bd5
StatusRuntimeException without stacktrace - Android compatibility
panchenko Dec 19, 2024
648672b
CR
panchenko Dec 20, 2024
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
12 changes: 5 additions & 7 deletions api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,11 @@ dependencies {
extension = "signature"
}
}
// TODO: Temporarily disabled until StatusException is fixed.
// Context: https://github.com/grpc/grpc-java/pull/11066
//signature (libraries.signature.android) {
// artifact {
// extension = "signature"
// }
//}
signature (libraries.signature.android) {
artifact {
extension = "signature"
}
}
}

tasks.named("javadoc").configure {
Expand Down
9 changes: 4 additions & 5 deletions api/src/main/java/io/grpc/InternalStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,11 @@ private InternalStatus() {}
public static final Metadata.Key<Status> CODE_KEY = Status.CODE_KEY;

/**
* Create a new {@link StatusRuntimeException} with the internal option of skipping the filling
* of the stack trace.
* Create a new {@link StatusRuntimeException} skipping the filling of the stack trace.
*/
@Internal
public static final StatusRuntimeException asRuntimeException(Status status,
Copy link
Member

Choose a reason for hiding this comment

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

Should we tweak the name here to make it clear at call sites it is special? This PR is an improvement as-is, so it is up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, a related question: is it important to keep the old signature deprecated? That's only for situations if some application has "api" and "core" from different versions, which is most likely unsupported but possible theoretically.

Copy link
Member

Choose a reason for hiding this comment

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

I do consider it at times, especially with lesser-used artifacts like grpclb or xds. But it is pretty rare to depend on api and not core (transitively). grpc-context-only usages do happen, but I don't expect we'd be saving many people much headache. I suspect there are pretty frequent internal breakages between api and core that nobody considers/notices.

I see now we could have kept the old API and just chose which implementation based on fillInStackTrace. But it's also the sort of thing "why would anyone use this API except to disable the stack trace?"

Although... we don't actually need InternalStatusRuntimeException in api any more, because it is just using public API. This is good enough. Let's get it in to fix animalsniffer and anything else can be followup if we so desire.

@Nullable Metadata trailers, boolean fillInStackTrace) {
return new StatusRuntimeException(status, trailers, fillInStackTrace);
public static StatusRuntimeException asRuntimeExceptionWithoutStacktrace(Status status,
@Nullable Metadata trailers) {
return new InternalStatusRuntimeException(status, trailers);
}
}
39 changes: 39 additions & 0 deletions api/src/main/java/io/grpc/InternalStatusRuntimeException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2015 The gRPC Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.grpc;

import javax.annotation.Nullable;

/**
* StatusRuntimeException without stack trace, implemented as a subclass, as the
* {@code String, Throwable, boolean, boolean} constructor is not available in the supported
* version of Android.
*
* @see StatusRuntimeException
*/
class InternalStatusRuntimeException extends StatusRuntimeException {
private static final long serialVersionUID = 0;

public InternalStatusRuntimeException(Status status, @Nullable Metadata trailers) {
super(status, trailers);
}

@Override
public synchronized Throwable fillInStackTrace() {
return this;
}
}
7 changes: 1 addition & 6 deletions api/src/main/java/io/grpc/StatusException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,7 @@ public StatusException(Status status) {
* @since 1.0.0
*/
public StatusException(Status status, @Nullable Metadata trailers) {
this(status, trailers, /*fillInStackTrace=*/ true);
}

StatusException(Status status, @Nullable Metadata trailers, boolean fillInStackTrace) {
super(Status.formatThrowableMessage(status), status.getCause(),
/* enableSuppression */ true, /* writableStackTrace */fillInStackTrace);
super(Status.formatThrowableMessage(status), status.getCause());
this.status = status;
this.trailers = trailers;
}
Expand Down
7 changes: 1 addition & 6 deletions api/src/main/java/io/grpc/StatusRuntimeException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,7 @@ public StatusRuntimeException(Status status) {
* @since 1.0.0
*/
public StatusRuntimeException(Status status, @Nullable Metadata trailers) {
this(status, trailers, /*fillInStackTrace=*/ true);
}

StatusRuntimeException(Status status, @Nullable Metadata trailers, boolean fillInStackTrace) {
super(Status.formatThrowableMessage(status), status.getCause(),
/* enable suppressions */ true, /* writableStackTrace */ fillInStackTrace);
super(Status.formatThrowableMessage(status), status.getCause());
this.status = status;
this.trailers = trailers;
}
Expand Down
8 changes: 0 additions & 8 deletions api/src/test/java/io/grpc/StatusExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,6 @@
@RunWith(JUnit4.class)
public class StatusExceptionTest {

@Test
public void internalCtorRemovesStack() {
StackTraceElement[] trace =
new StatusException(Status.CANCELLED, null, false) {}.getStackTrace();

assertThat(trace).isEmpty();
}

@Test
public void normalCtorKeepsStack() {
StackTraceElement[] trace =
Expand Down
2 changes: 1 addition & 1 deletion api/src/test/java/io/grpc/StatusRuntimeExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class StatusRuntimeExceptionTest {
@Test
public void internalCtorRemovesStack() {
StackTraceElement[] trace =
new StatusRuntimeException(Status.CANCELLED, null, false) {}.getStackTrace();
new InternalStatusRuntimeException(Status.CANCELLED, null) {}.getStackTrace();

assertThat(trace).isEmpty();
}
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/io/grpc/internal/ServerCallImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,10 +373,10 @@ private void closedInternal(Status status) {
} else {
call.cancelled = true;
listener.onCancel();
// The status will not have a cause in all failure scenarios but we want to make sure
// The status will not have a cause in all failure scenarios, but we want to make sure
// we always cancel the context with one to keep the context cancelled state consistent.
cancelCause = InternalStatus.asRuntimeException(
Status.CANCELLED.withDescription("RPC cancelled"), null, false);
cancelCause = InternalStatus.asRuntimeExceptionWithoutStacktrace(
Status.CANCELLED.withDescription("RPC cancelled"), null);
}
} finally {
// Cancel context after delivering RPC closure notification to allow the application to
Expand Down
4 changes: 2 additions & 2 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 @@ -887,8 +887,8 @@ private void closedInternal(final Status status) {
// failed status has an exception we will create one here if needed.
Throwable cancelCause = status.getCause();
if (cancelCause == null) {
cancelCause = InternalStatus.asRuntimeException(
Status.CANCELLED.withDescription("RPC cancelled"), null, false);
cancelCause = InternalStatus.asRuntimeExceptionWithoutStacktrace(
Status.CANCELLED.withDescription("RPC cancelled"), null);
}

// The callExecutor might be busy doing user work. To avoid waiting, use an executor that
Expand Down
Loading