5.0.0 to 5.1.4 changes status exception behaviour

[m-miyano]

Hello,

I tried to update from v5.0.0 to v5.1.4, I observed a change in the status code handling when an exception occurs.

My service uses streaming. Here's a snippet of the relevant code:

service MyService {
    rpc Streaming(stream MyRequest) returns (stream MyResponse) {}
}

In the context of onFailure, pass StatusRuntimeException to observer.onError.
simplified code snippet below:

override fun streaming(responseObserver: StreamObserver<MyResponse>): StreamObserver<MyRequest> {
    override fun onCompleted() {
        runCatching {
            // some processing
        }.onFailure {
            responseObserver.onError(
                Status.NOT_FOUND
                    .withCause(Exception("some exception"))
                    .asRuntimeException()) //to StatusRuntimeException
        }
    }
}

In v5.0.0

ERROR:
  Code: NOT_FOUND

In v5.1.4

ERROR:
  Code: INTERNAL

The issue below mentions a problem with StatusException, but the issue also occurs with StatusRuntimeException.
#371

It appears that the newly introduced passage through the failureHandlingSupport.closeCall method is causing the impact.

grpc-spring-boot-starter/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/recovery/GRpcExceptionHandlerInterceptor.java

Lines 48 to 53 in b6f9a75

	Boolean handled = Optional.ofNullable(EXCEPTION_HANDLED.get())
	.map(AtomicBoolean::get)
	.orElse(!exceptionHandled.compareAndSet(false, true));
	if(null != status.getCause() && !handled){
	failureHandlingSupport.closeCall(new GRpcRuntimeExceptionWrapper(status.getCause()), this, trailers);
	}

If I've implemented something contrary to your intentions, please let me know.
Thank you always for your support.

[m-miyano]

In this case, the GRpcRuntimeExceptionWrapper wraps Exception, not StatusRuntimeException.

This is a debugging infomation for the following section:

grpc-spring-boot-starter/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/FailureHandlingSupport.java

Lines 28 to 29 in b6f9a75

	public void closeCall(Exception e, ServerCall<?, ?> call, Metadata headers, Consumer<GRpcExceptionScope.GRpcExceptionScopeBuilder> customizer) throws RuntimeException {
	Optional.ofNullable(EXCEPTION_HANDLED.get()).ifPresent(h -> h.set(true));

e = {GRpcRuntimeExceptionWrapper@12662} "org.lognet.springboot.grpc.recovery.GRpcRuntimeExceptionWrapper"
 hint = null
 backtrace = {Object[6]@12670} 
 detailMessage = null
 cause = {Exception@12671} "java.lang.Exception: some exception"
 stackTrace = {StackTraceElement[31]@12675} 
 depth = 31
 suppressedExceptions = {Collections$EmptyList@12673}  size = 0

[jvmlet]

How do I reproduce this ? Just throw new StatusRuntimeException() from the method ?

[m-miyano]

Here is a minimal project that helps to reproduce the issue.

I found that the interceptor works and the behavior changes when org.springframework.boot:spring-boot-starter-validation is added to the dependencies.

[jvmlet]

@m-miyano, I've added more test to mimic your scenario, please try to upgrade to 5.1.5-SNAPSHOT :

repositories {
  mavenCentral()
  maven { url "https://oss.sonatype.org/content/repositories/snapshots" } // for snapshot builds
}
dependencies {
  implementation 'io.github.lognet:grpc-spring-boot-starter:5.1.5-SNAPSHOT'
}

[m-miyano]

@jvmlet Thank you for your assistance.
Unfortunately, the issue has not been resolved.

In v5.0.0

ERROR:
  Code: NOT_FOUND

In v5.1.4

ERROR:
  Code: INTERNAL

In v5.1.5-SNAPSHOT

ERROR:
  Code: INTERNAL

Here is a project that try to upgrade to v5.1.5-SNAPSHOT.

[jvmlet]

Thanks @m-miyano , I will add streaming case as well.

[jorgerod]

Hi @jvmlet @m-miyano

I have the same problem. Is there any news?

Are there any workarounds?

Thank you, very much

[jvmlet]

@jorgerod , you mean you have bidi-stream case that fails ?

[jorgerod]

Hi @jvmlet

This case fails when cause is added to exception and as @m-miyano says when there is the org.hibernate.validator:hibernate-validator dependency (spring-boot-starter-validation brings it transitively).

This controller return: Status code: 13 INTERNAL

@GRpcService
public class MyController extends ProductEndPointGrpc.ProductEndPointImplBase {


    @Override
    public void findAllProducts(Empty request, StreamObserver<Service.ProductResponseList> responseObserver) {
        
        responseObserver.onError(Status.NOT_FOUND
                .withCause(new IllegalArgumentException())
                .asRuntimeException());
    }
}

And without withCause return: Status code: 5 NOT_FOUND

@GRpcService
public class MyController extends ProductEndPointGrpc.ProductEndPointImplBase {


    @Override
    public void findAllProducts(Empty request, StreamObserver<Service.ProductResponseList> responseObserver) {
        responseObserver.onError(Status.NOT_FOUND
                .asRuntimeException());
    }
}

[alicanhaman]

The issue still persists on version 5.1.5. Removing the cause is a bandaid fix for now. Is there any plans to fix this in the next version?

[debraj-manna]

We are also facing the same problem. We are also waiting for the fix

[berksean]

Just chiming in here and that this issue seems to come into play only when the Error Handling functionality is in use (@GrpcAdvice/@GrpcErrorHandler).

We don't directly use it, but it appears like spring autoconfiguration is creating a default handler whenever the jakarta.validation.Validator class is present (handler is registered for the ConstraintViolationException). We happened to be getting the dependency unnecessarily, and were able to fix our issue in some instances by removing the dependency...

It would be helpful if there were a simple way to disable this bean, or the GRpcValidationConfiguration entirely. Right now it's brought in as part of auto configuration and is only conditional on the presence of the Validator class, so it's hard to exclude this behavior if we don't need the starter's validation facilities, but do need the jakarta validator for other reasons.

Clearly allowing this to work as expected would be preferred. Perhaps by passing in an "unhandled status" to the failureHandlingSupport method that should be returned instead of defaulting to bare INTERNAL. Calling code in GRpcExceptionHandlerInterceptor could pass the status it was provided and the failureHandlingSupport could return that instead.

[berksean]

Also, one possible approach to fix the behavior is a server interceptor that conditionally manipulates the status. This mostly keeps the custom error handling facility and avoids needing to strip the cause (though it may result in some extra nested cause chains.:

In kotlin --

@Component
// We want this interceptor to adjust any statuses seen by all interceptors, and thus be as close to the application
// side of request handling as possible. An explicit LOWEST_PRECEDENCE value will place it as close to the
// application handling logic as possible.
@GRpcGlobalInterceptor
@Order(Ordered.LOWEST_PRECEDENCE)
class LogNetStatusHandlingFixInterceptor : ServerInterceptor {

    override fun <ReqT : Any, RespT : Any> interceptCall(
        serverCall: ServerCall<ReqT, RespT>,
        metadata: Metadata,
        serverCallHandler: ServerCallHandler<ReqT, RespT>,
    ): ServerCall.Listener<ReqT> {
        return serverCallHandler.startCall(
            object : SimpleForwardingServerCall<ReqT, RespT>(serverCall) {
                override fun close(
                    status: Status?,
                    trailers: Metadata?,
                ) {
                    super.close(status?.withAdjustedCause(), trailers)
                }
            },
            metadata,
        )
    }

    companion object {
        private val logger = KotlinLogging.logger {}

        /**
         * @return a [Status] where the [cause] is [this] status as an exception if necessary to avoid the LogNet
         * starter's handling behavior.
         */
        private fun Status.withAdjustedCause() =
            // If the status has no cause specified, or has a signature similar to the status returned when an unhandled
            // exception is caught, then simply pass through the status. In the null case, the lognet error handling
            // doesn't come into play. In the unhandled status condition, lognet error handlers should have a chance to
            // perform custom handling.
            if (cause == null || (code == Status.Code.UNKNOWN && description.isNullOrBlank())) {
                this
            } else {
                withCause(this.asException())
            }
    }
}

[juliojgd]

@jvmlet any thoughts/advances on this issue?