core: remove potential this-escapes by extracting constructions of StatusException and StatusRuntimeException to builders

[tharakadesilva]

With JDK 21, this-escape issues are now being flagged by the compiler: https://bugs.openjdk.org/browse/JDK-8299995 by default and we started seeing these errors in our project:

external/io_grpc_grpc_java/api/src/main/java/io/grpc/StatusRuntimeException.java:50: warning: [this-escape] possible 'this' escape before subclass is fully initialized
    this(status, trailers, /*fillInStackTrace=*/ true);
        ^
external/io_grpc_grpc_java/api/src/main/java/io/grpc/StatusRuntimeException.java:58: warning: [this-escape] previous possible 'this' escape happens here via invocation
    fillInStackTrace();
                    ^
external/io_grpc_grpc_java/api/src/main/java/io/grpc/StatusException.java:48: warning: [this-escape] possible 'this' escape before subclass is fully initialized
    this(status, trailers, /*fillInStackTrace=*/ true);
        ^
external/io_grpc_grpc_java/api/src/main/java/io/grpc/StatusException.java:56: warning: [this-escape] previous possible 'this' escape happens here via invocation
    fillInStackTrace();

This solution is changing the bean creation to a builder pattern and moving the fillInStackTrace invocation happening in the constructor to the build function.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: tharakadesilva / name: Tharaka De Silva (ced1784, eaed8c9, c265a5a)

[ejona86]

I think it'd probably be better to remove the fillInStackTrace stuff. It doesn't look used. If we end up using it again (it does have some advantages) then we'd extend the exception and make it unconditional in that new class.

What is the this leakage for this(status, trailers, /*fillInStackTrace=*/ true); ? I don't see how it happens.

[ejona86]

There's no way we can delete this constructor. This is stable API.

[ejona86]

@tharakadesilva, are you still interested in this change?

[tharakadesilva]

Hi @ejona86 thanks for the feedback and sorry for the delay in response.

I think it'd probably be better to remove the fillInStackTrace stuff. It doesn't look used. If we end up using it again (it does have some advantages) then we'd extend the exception and make it unconditional in that new class.

Even if we move to an unconditional class we would have the same issue if we call the function from the constructor unless we make that class final.

What is the this leakage for this(status, trailers, /fillInStackTrace=/ true); ? I don't see how it happens.

It is only a possibility (or this might be a false positive). Here's my understanding (please bear with me in case this is wrong):

Let's take a sub-class like this:

public class MyException extends StatusException {
    private final AtomicInteger count;

    public MyException(@Nonnull final AtomicInteger count) {
        super();
        this.count = count;
    }

    @Override
    public void fillInStackTrace() {
        super.fillInStackTrace();
        count.increment();
    }
}

1. We create a MyException object by passing an AtomicInteger.
2. The constructor calls super constructor.
3. Super constructor calls fillInStackTrace (from MyException coz it is overridden).
4. It tries to do count.increment() but count is still null.

---

There's no way we can delete this constructor. This is stable API.

What would you recommend?

@tharakadesilva, are you still interested in this change?

I am. For now, I have a Bazel patch override with @SuppressWarning.

[panchenko]

I think we can use the super constructor with 4 parameters which was introduced in java 1.7

diff --git a/api/src/main/java/io/grpc/StatusRuntimeException.java b/api/src/main/java/io/grpc/StatusRuntimeException.java
index 68b816fc7..70c4d10f0 100644
--- a/api/src/main/java/io/grpc/StatusRuntimeException.java
+++ b/api/src/main/java/io/grpc/StatusRuntimeException.java
@@ -29,8 +29,6 @@ public class StatusRuntimeException extends RuntimeException {
   private final Status status;
   private final Metadata trailers;
 
-  private final boolean fillInStackTrace;
-
   /**
    * Constructs the exception with both a status. See also {@link Status#asRuntimeException()}.
    *
@@ -51,21 +49,10 @@ public class StatusRuntimeException extends RuntimeException {
   }
 
   StatusRuntimeException(Status status, @Nullable Metadata trailers, boolean fillInStackTrace) {
-    super(Status.formatThrowableMessage(status), status.getCause());
+    super(Status.formatThrowableMessage(status), status.getCause(),
+            /* enableSuppression */ true, /* writableStackTrace */ fillInStackTrace);
     this.status = status;
     this.trailers = trailers;
-    this.fillInStackTrace = fillInStackTrace;
-    fillInStackTrace();
-  }
-
-  @Override
-  public synchronized Throwable fillInStackTrace() {
-    // Let's observe final variables in two states!  This works because Throwable will invoke this
-    // method before fillInStackTrace is set, thus doing nothing.  After the constructor has set
-    // fillInStackTrace, this method will properly fill it in.  Additionally, sub classes may call
-    // this normally, because fillInStackTrace will either be set, or this method will be
-    // overriden.
-    return fillInStackTrace ? super.fillInStackTrace() : this;
   }

[panchenko]

created as #11064

[tharakadesilva]

Closing in lieu of @panchenko's MR, thank you both!!