This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pekko-grpc.git
The following commit(s) were added to refs/heads/main by this push:
new 3dd4c682 Do not return NotImplementedError messages to the caller
(#874)
3dd4c682 is described below
commit 3dd4c68219394fe2f64da86702e309063ea092e1
Author: PJ Fanning <[email protected]>
AuthorDate: Tue Sep 1 10:07:48 2026 +0100
Do not return NotImplementedError messages to the caller (#874)
Motivation:
`defaultMapper` logged and scrubbed the message of every failure it did not
recognise, but two cases were different:
case e: NotImplementedError =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
case e: UnsupportedOperationException =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
Those messages come from whatever the service implementation threw, so an
exception raised deep inside a handler put its text on the wire. Neither
case was
logged either, so the detail went to the caller and nowhere else - the
opposite
of every other branch.
`???` in Scala throws `NotImplementedError`, and
`UnsupportedOperationException`
comes out of many collection and stream operations, so these are easy to hit
without meaning to report anything.
Modification:
Both cases now log at warning and answer with a bare `UNIMPLEMENTED`,
matching
how the mapper treats every other unrecognised failure, in both the
scaladsl and
javadsl handlers.
`GrpcServiceException` remains the way to send a description deliberately,
so the
generated handlers now raise that for an unknown method rather than relying
on
the message of a `NotImplementedError` reaching the caller. The behaviour a
client sees for an unknown method is unchanged.
Result:
An accidental `???` or unsupported collection operation no longer describes
itself to the caller, and the detail is logged where an operator can see it.
Deliberate descriptions still work.
Tests:
- 3 new cases in `GrpcExceptionHandlerSpec`: the message of each exception
type
is withheld, and a deliberate `GrpcServiceException` description still
arrives
- The existing table-driven expectation is updated, which is the behaviour
change
- The existing end-to-end `ErrorReportingSpec` in both plugin-testers
already
asserts a client calling an unknown method receives
`grpc-message: Not implemented: UnknownMethod`; both still pass, which is
what
covers the codegen half of this
- Confirmed the guard bites: restoring the leak fails 3 tests
- sbt "runtime/testOnly ...GrpcExceptionHandlerSpec" - 29 passed
- sbt "plugin-tester-scala/testOnly ...ErrorReportingSpec" - 2 passed
- sbt "plugin-tester-java/testOnly ...ErrorReportingSpec" - 2 passed
- sbt "runtime/mimaReportBinaryIssues" - passed
- sbt scalafmtAll scalafmtSbt - applied
- sbt "runtime/test" - not run locally, left to CI
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
.../twirl/templates/JavaServer/Handler.scala.txt | 2 +-
.../twirl/templates/ScalaServer/Handler.scala.txt | 4 +--
.../pekko/grpc/javadsl/GrpcExceptionHandler.scala | 16 +++++++---
.../pekko/grpc/scaladsl/GrpcExceptionHandler.scala | 16 +++++++---
.../grpc/scaladsl/GrpcExceptionHandlerSpec.scala | 36 ++++++++++++++++++++--
5 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/codegen/src/main/twirl/templates/JavaServer/Handler.scala.txt
b/codegen/src/main/twirl/templates/JavaServer/Handler.scala.txt
index 18194324..3f2ad445 100644
--- a/codegen/src/main/twirl/templates/JavaServer/Handler.scala.txt
+++ b/codegen/src/main/twirl/templates/JavaServer/Handler.scala.txt
@@ -218,7 +218,7 @@ public class @{serviceName}HandlerFactory {
}
default:
CompletableFuture<org.apache.pekko.http.javadsl.model.HttpResponse> result =
new CompletableFuture<>();
- result.completeExceptionally(new
UnsupportedOperationException("Not implemented: " + method));
+ result.completeExceptionally(new
org.apache.pekko.grpc.GrpcServiceException(io.grpc.Status.UNIMPLEMENTED.withDescription("Not
implemented: " + method)));
response = result;
}
return response.exceptionally(e -> GrpcExceptionHandler.standard(e,
eHandler, writer, system));
diff --git a/codegen/src/main/twirl/templates/ScalaServer/Handler.scala.txt
b/codegen/src/main/twirl/templates/ScalaServer/Handler.scala.txt
index 130c663a..d735d657 100644
--- a/codegen/src/main/twirl/templates/ScalaServer/Handler.scala.txt
+++ b/codegen/src/main/twirl/templates/ScalaServer/Handler.scala.txt
@@ -140,7 +140,7 @@ object @{serviceName}Handler {
.recoverWith(GrpcExceptionHandler.from(eHandler(system.classicSystem))(system,
writer))
}
}
- case m =>
GrpcExceptionHandler.from(eHandler(system.classicSystem))(system, writer)(new
NotImplementedError(s"Not implemented: $m"))
+ case m =>
GrpcExceptionHandler.from(eHandler(system.classicSystem))(system, writer)(new
pekko.grpc.GrpcServiceException(io.grpc.Status.UNIMPLEMENTED.withDescription(s"Not
implemented: $m")))
}
).getOrElse(unsupportedMediaType)
@@ -185,7 +185,7 @@ object @{serviceName}Handler {
.recoverWith(GrpcExceptionHandler.from(eHandler(system.classicSystem))(system,
writer))
}
}
- case m =>
GrpcExceptionHandler.from(eHandler(system.classicSystem))(system, writer)(new
NotImplementedError(s"Not implemented: $m"))
+ case m =>
GrpcExceptionHandler.from(eHandler(system.classicSystem))(system, writer)(new
pekko.grpc.GrpcServiceException(io.grpc.Status.UNIMPLEMENTED.withDescription(s"Not
implemented: $m")))
}
).getOrElse(unsupportedMediaType)
diff --git
a/runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcExceptionHandler.scala
b/runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcExceptionHandler.scala
index e3287087..05ef05f2 100644
---
a/runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcExceptionHandler.scala
+++
b/runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcExceptionHandler.scala
@@ -35,6 +35,7 @@ import scala.concurrent.ExecutionException
object GrpcExceptionHandler {
private val INTERNAL = Trailers(Status.INTERNAL)
private val INVALID_ARGUMENT = Trailers(Status.INVALID_ARGUMENT)
+ private val UNIMPLEMENTED = Trailers(Status.UNIMPLEMENTED)
def defaultMapper: jFunction[ActorSystem, jFunction[Throwable, Trailers]] =
new jFunction[ActorSystem, jFunction[Throwable, Trailers]] {
@@ -58,10 +59,17 @@ object GrpcExceptionHandler {
else default(system)(e.getCause)
case grpcException: GrpcServiceException =>
Trailers(grpcException.status, grpcException.metadata)
case _: MissingParameterException => INVALID_ARGUMENT
- case e: NotImplementedError =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
- case e: UnsupportedOperationException =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
- case e: StatusRuntimeException => Trailers(e.getStatus,
new GrpcMetadataImpl(e.getTrailers))
- case e: PeerClosedStreamException =>
+ case e: NotImplementedError =>
+ // the message is whatever the service implementation happened to
throw with, so it
+ // is logged rather than returned. Throw a GrpcServiceException to
send a description
+ // on purpose; that is what the generated handlers do for an
unknown method.
+ log(system).warning(e, "Unimplemented: [{}]", e.getMessage)
+ UNIMPLEMENTED
+ case e: UnsupportedOperationException =>
+ log(system).warning(e, "Unimplemented: [{}]", e.getMessage)
+ UNIMPLEMENTED
+ case e: StatusRuntimeException => Trailers(e.getStatus, new
GrpcMetadataImpl(e.getTrailers))
+ case e: PeerClosedStreamException =>
log(system).warning(e, "Peer closed the stream: [{}]",
e.getMessage)
INTERNAL
case other =>
diff --git
a/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandler.scala
b/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandler.scala
index bf84958f..ab60d298 100644
---
a/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandler.scala
+++
b/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandler.scala
@@ -31,6 +31,7 @@ import pekko.event.Logging
object GrpcExceptionHandler {
private val INTERNAL = Trailers(Status.INTERNAL)
private val INVALID_ARGUMENT = Trailers(Status.INVALID_ARGUMENT)
+ private val UNIMPLEMENTED = Trailers(Status.UNIMPLEMENTED)
private def log(system: ActorSystem) = Logging(system,
"org.apache.pekko.grpc.scaladsl.GrpcExceptionHandler")
@@ -39,10 +40,17 @@ object GrpcExceptionHandler {
if (e.getCause == null) INTERNAL
else defaultMapper(system)(e.getCause)
case grpcException: GrpcServiceException => Trailers(grpcException.status,
grpcException.metadata)
- case e: NotImplementedError =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
- case e: UnsupportedOperationException =>
Trailers(Status.UNIMPLEMENTED.withDescription(e.getMessage))
- case _: MissingParameterException => INVALID_ARGUMENT
- case e: StatusRuntimeException =>
+ case e: NotImplementedError =>
+ // the message is whatever the service implementation happened to throw
with, so it is
+ // logged rather than returned. Throw a GrpcServiceException to send a
description on
+ // purpose; that is what the generated handlers do for an unknown method.
+ log(system).warning(e, "Unimplemented: [{}]", e.getMessage)
+ UNIMPLEMENTED
+ case e: UnsupportedOperationException =>
+ log(system).warning(e, "Unimplemented: [{}]", e.getMessage)
+ UNIMPLEMENTED
+ case _: MissingParameterException => INVALID_ARGUMENT
+ case e: StatusRuntimeException =>
val meta = Option(e.getTrailers).getOrElse(new io.grpc.Metadata())
Trailers(e.getStatus, new GrpcMetadataImpl(meta))
case e: PeerClosedStreamException =>
diff --git
a/runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandlerSpec.scala
b/runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandlerSpec.scala
index 92936b04..553de1a6 100644
---
a/runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandlerSpec.scala
+++
b/runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/GrpcExceptionHandlerSpec.scala
@@ -41,9 +41,11 @@ class GrpcExceptionHandlerSpec extends AnyWordSpec with
Matchers with ScalaFutur
else expected(e.getCause)
case grpcException: GrpcServiceException => grpcException.status
case e: StatusRuntimeException => e.getStatus
- case e: NotImplementedError =>
Status.UNIMPLEMENTED.withDescription(e.getMessage)
- case e: UnsupportedOperationException =>
Status.UNIMPLEMENTED.withDescription(e.getMessage)
- case _ => Status.INTERNAL
+ // the message is not returned to the caller: it is whatever the service
implementation
+ // happened to throw with, so it is logged instead
+ case _: NotImplementedError => Status.UNIMPLEMENTED
+ case _: UnsupportedOperationException => Status.UNIMPLEMENTED
+ case _ => Status.INTERNAL
}
val otherTypes: Seq[Throwable] = Seq(
@@ -70,6 +72,34 @@ class GrpcExceptionHandlerSpec extends AnyWordSpec with
Matchers with ScalaFutur
}
}
+ "defaultMapper" should {
+
+ "not return the message of a NotImplementedError to the caller" in {
+ val status = defaultMapper(system)(new NotImplementedError("internal
detail")).status
+
+ status.getCode shouldBe Status.UNIMPLEMENTED.getCode
+ status.getDescription shouldBe null
+ }
+
+ "not return the message of an UnsupportedOperationException to the caller"
in {
+ val status = defaultMapper(system)(new
UnsupportedOperationException("internal detail")).status
+
+ status.getCode shouldBe Status.UNIMPLEMENTED.getCode
+ status.getDescription shouldBe null
+ }
+
+ "still return a description that was set deliberately" in {
+ // a GrpcServiceException is how a service says what to tell the caller,
and is what the
+ // generated handlers raise for an unknown method
+ val deliberate = new
GrpcServiceException(Status.UNIMPLEMENTED.withDescription("Not implemented:
SayHello"))
+
+ val status = defaultMapper(system)(deliberate).status
+
+ status.getCode shouldBe Status.UNIMPLEMENTED.getCode
+ status.getDescription shouldBe "Not implemented: SayHello"
+ }
+ }
+
"default(defaultMapper)" should {
(otherTypes ++ executionExceptions).foreach { e =>
s"Correctly map $e" in {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
