AssertionErrors are converted into service exceptions with type internal #727
Conversation
At the moment, AssertionErrors are not handled by remoting, even though they're used fairly widely within services and libraries we have internally. This means that the body ends up being empty if they ever get hit, which leads to a fairly cryptic exception being thrown on the client. Bugs are still debuggable, it's just harder. This PR makes the claim that AssertionErrors should be handled specially. Could also argue that all errors should be handled the same as e.g. a random RuntimeException (specifically thinking NoSuchMethodError).
| import javax.ws.rs.ext.Provider; | ||
|
|
||
| @Provider | ||
| final class AssertionErrorExceptionMapper extends JsonExceptionMapper<AssertionError> { |
There was a problem hiding this comment.
It does seem pretty obscure that people are throwing AssertionErrors - I've generally only seen them in private constructors (throw new AssertionError("Not instantiable");). However, I think it does makes sense that in the strange case that this happens, we might as well give clients something helpful.
Can we expand it to all Errors? This would get us NoSuchFieldError, NoSuchMethodError and NoClassDefFoundError?
There was a problem hiding this comment.
Expanding to Error or Throwable would also cover the occasional OutOfMemoryError
There was a problem hiding this comment.
yeah that was my reticence at the start. I guess there's the thing where we hope that OOMs get handled by the JVM itself?
|
I have rewritten to be in terms of the above - aka all Throwables - since sls-packaging handles throwing on out-of-memory. |
|
This should be fine because we have |
* Expose HostMetricsRegistry record methods (#780) This is so that we can separately implement a HostMetricsSink for it (see #779) such that we can share host metrics when constructing both conjure and remoting3 clients * publish BOM for jaxrs-client (#789) * Excavator: Render CircleCI file using template specified in .circleci/template.sh (#791) * Upgrade OkHttp to 3.11.0. (#794) * AssertionErrors are converted into service exceptions with type internal (#727) * No more ThreadLocals, delegate everything to 'palantir/tracing-java' (#799) * Use BINTRAY_*_REMOTING envs (#802) The project's default bintray creds are currently set up to publish to `conjure-java-runtime`. Use these custom env vars to maintain ability to publish http-remoting. * Better behaviour in the presence of 429s (#786)
At the moment, AssertionErrors are not handled by remoting, even though
they're used fairly widely within services and libraries we have
internally. This means that the body ends up being empty if they
ever get hit, which leads to a fairly cryptic exception being
thrown on the client.
Bugs are still debuggable, it's just harder. This PR makes the claim
that AssertionErrors should be handled specially.
Could also argue that all errors should be handled the same as e.g.
a random RuntimeException (specifically thinking NoSuchMethodError).