Catch generic throwable and pass exception to returned future #1285
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
| @@ -285,6 +285,9 @@ public ListenableFuture<WriteApiResponse> writeJsonStream( | |||
| writeApiFutureResponse.setException(e); | |||
| // Restore interrupted state in case of an InterruptedException | |||
| Thread.currentThread().interrupt(); | |||
| } catch (Throwable t) { | |||
There was a problem hiding this comment.
@elefeint Catching a wider exception is a good idea here and thanks for making the changes. However shall we catch Exception instead? Catching Throwable results in this Sonar issue: https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&pullRequest=1285&id=GoogleCloudPlatform_spring-cloud-gcp&open=AYPCUFuggImngqXU1xdj
There was a problem hiding this comment.
Sonar is right, in general, however the user experience will be significantly worse if we follow this recommendation -- for example, if an unexpected Exception (or even Throwable such as linkage errors) arises during the separate thread execution, the error will just disappear without leaving a trace.
There was a problem hiding this comment.
That causes 2 issues:
- The error disappears without any user-visible evidence.
CompletableFuturewill be left hanging forever.
This PR adds a catch-all for any unforeseen errors and passes the error to the returned future.
In normal code, catching
Throwableis a bad idea -- the error should be allowed to propagate and stop the app. However, when running in a separate thread, an uncaughtThrowablewill simply disappear without a trace, so we have to catch it and hand the error over to the returned future, so that the failure may be acted upon.Unrelated changes: