Catch (Runtime)Exceptions instead of Throwables
Throwables are a very broad superset of problems,
including Java 'Error's. Catching them is discouraged [1]
because they include problems that should halt the JVM
(generally subclasses of the 'Error' class).
This commit catches Exception and RuntimeException instead
where possible.
[1] https://stackoverflow.com/questions/24344511/why-is-catching-a-runtimeexception-not-considered-a-good-programming-practice
Change-Id: I6b41b14af798bf19cc97fee9f59e717aa12a9cda
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 7ddf2ba..f476566 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -369,11 +369,11 @@
if (commonServer != null) {
try {
commonServer.close();
- } catch (Throwable t) {
+ } catch (Exception e) {
throw new AssertionError(
"Error stopping common server in "
+ (firstTest != null ? firstTest.getTestClass().getName() : "unknown test class"),
- t);
+ e);
} finally {
commonServer = null;
}
diff --git a/java/com/google/gerrit/lifecycle/LifecycleManager.java b/java/com/google/gerrit/lifecycle/LifecycleManager.java
index 4f09a09..42123d7 100644
--- a/java/com/google/gerrit/lifecycle/LifecycleManager.java
+++ b/java/com/google/gerrit/lifecycle/LifecycleManager.java
@@ -107,7 +107,7 @@
LifecycleListener obj = listeners.get(i).get();
try {
obj.stop();
- } catch (Throwable err) {
+ } catch (RuntimeException err) {
logger.atWarning().withCause(err).log("Failed to stop %s", obj.getClass());
}
startedIndex = i - 1;
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 2b4cfef..a3605f7 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -310,7 +310,7 @@
RuntimeShutdown.waitFor();
}
return 0;
- } catch (Throwable err) {
+ } catch (RuntimeException err) {
logger.atSevere().withCause(err).log("Unable to start daemon");
return 1;
}
diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
index 89b4228..6f3514f 100644
--- a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
+++ b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
@@ -547,7 +547,7 @@
filterHolder.setInitParameters(initParams);
}
app.addFilter(filterHolder, "/*", EnumSet.of(DispatcherType.REQUEST, DispatcherType.ASYNC));
- } catch (Throwable e) {
+ } catch (Exception e) {
throw new IllegalArgumentException(
"Unable to instantiate front-end HTTP Filter " + filterClassName, e);
}
diff --git a/java/com/google/gerrit/server/RequestCleanup.java b/java/com/google/gerrit/server/RequestCleanup.java
index f405c57..e07d148 100644
--- a/java/com/google/gerrit/server/RequestCleanup.java
+++ b/java/com/google/gerrit/server/RequestCleanup.java
@@ -44,7 +44,7 @@
for (Iterator<Runnable> i = cleanup.iterator(); i.hasNext(); ) {
try {
i.next().run();
- } catch (Throwable err) {
+ } catch (Exception err) {
logger.atSevere().withCause(err).log("Failed to execute per-request cleanup");
}
i.remove();
diff --git a/java/com/google/gerrit/server/change/AbandonUtil.java b/java/com/google/gerrit/server/change/AbandonUtil.java
index 1bc1fad..d030ec1 100644
--- a/java/com/google/gerrit/server/change/AbandonUtil.java
+++ b/java/com/google/gerrit/server/change/AbandonUtil.java
@@ -93,7 +93,7 @@
try {
batchAbandon.batchAbandon(updateFactory, project, internalUser, changes, message);
count += changes.size();
- } catch (Throwable e) {
+ } catch (Exception e) {
StringBuilder msg = new StringBuilder("Failed to auto-abandon inactive change(s):");
for (ChangeData change : changes) {
msg.append(" ").append(change.getId().get());
diff --git a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
index cae213f..2823548 100644
--- a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
+++ b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
@@ -145,7 +145,7 @@
}
groupUuids = newGroupUuids;
logger.atInfo().log("Run group indexer, %s groups reindexed", reindexCounter);
- } catch (Throwable t) {
+ } catch (Exception t) {
logger.atSevere().withCause(t).log("Failed to reindex groups");
}
}
diff --git a/java/com/google/gerrit/server/logging/PerformanceLogContext.java b/java/com/google/gerrit/server/logging/PerformanceLogContext.java
index b6dafdc..65e033b15 100644
--- a/java/com/google/gerrit/server/logging/PerformanceLogContext.java
+++ b/java/com/google/gerrit/server/logging/PerformanceLogContext.java
@@ -92,7 +92,7 @@
p -> {
try (TraceContext traceContext = newPluginTrace(p)) {
performanceLogRecords.forEach(r -> r.writeTo(p.get()));
- } catch (Throwable e) {
+ } catch (RuntimeException e) {
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", p.get().getClass(), p.getPluginName());
}
diff --git a/java/com/google/gerrit/server/plugincontext/PluginContext.java b/java/com/google/gerrit/server/plugincontext/PluginContext.java
index 90d56c8..1cfee65 100644
--- a/java/com/google/gerrit/server/plugincontext/PluginContext.java
+++ b/java/com/google/gerrit/server/plugincontext/PluginContext.java
@@ -204,7 +204,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionImplConsumer.run(extensionImpl);
- } catch (Throwable e) {
+ } catch (Exception e) {
pluginMetrics.incrementErrorCount(extension);
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
@@ -233,7 +233,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionConsumer.run(extension);
- } catch (Throwable e) {
+ } catch (Exception e) {
pluginMetrics.incrementErrorCount(extension);
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
@@ -267,7 +267,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionImplConsumer.run(extensionImpl);
- } catch (Throwable e) {
+ } catch (Exception e) {
Throwables.throwIfInstanceOf(e, exceptionClass);
Throwables.throwIfUnchecked(e);
pluginMetrics.incrementErrorCount(extension);
@@ -304,7 +304,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionConsumer.run(extension);
- } catch (Throwable e) {
+ } catch (Exception e) {
Throwables.throwIfInstanceOf(e, exceptionClass);
Throwables.throwIfUnchecked(e);
pluginMetrics.incrementErrorCount(extension);
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 0a06081..8d17d85 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -253,7 +253,7 @@
FileSnapshot snapshot = FileSnapshot.save(off.toFile());
Plugin offPlugin = loadPlugin(name, off, snapshot);
disabled.put(name, offPlugin);
- } catch (Throwable e) {
+ } catch (Exception e) {
// This shouldn't happen, as the plugin was loaded earlier.
logger.atWarning().withCause(e.getCause()).log(
"Cannot load disabled plugin %s", active.getName());
@@ -510,7 +510,7 @@
if (!newPlugin.isDisabled()) {
try {
newPlugin.start(env);
- } catch (Throwable e) {
+ } catch (Exception e) {
newPlugin.stop(env);
throw e;
}
@@ -528,7 +528,7 @@
}
broken.remove(name);
return newPlugin;
- } catch (Throwable err) {
+ } catch (Exception err) {
broken.put(name, snapshot);
throw new PluginInstallException(err);
}
diff --git a/java/com/google/gerrit/server/rules/PrologEnvironment.java b/java/com/google/gerrit/server/rules/PrologEnvironment.java
index 1a563ad..7d626da 100644
--- a/java/com/google/gerrit/server/rules/PrologEnvironment.java
+++ b/java/com/google/gerrit/server/rules/PrologEnvironment.java
@@ -143,7 +143,7 @@
for (Iterator<Runnable> i = cleanup.iterator(); i.hasNext(); ) {
try {
i.next().run();
- } catch (Throwable err) {
+ } catch (Exception err) {
logger.atSevere().withCause(err).log("Failed to execute cleanup for PrologEnvironment");
}
i.remove();
diff --git a/java/com/google/gerrit/server/update/RetryableChangeAction.java b/java/com/google/gerrit/server/update/RetryableChangeAction.java
index 152db2c..84ec2bb 100644
--- a/java/com/google/gerrit/server/update/RetryableChangeAction.java
+++ b/java/com/google/gerrit/server/update/RetryableChangeAction.java
@@ -82,11 +82,11 @@
public T call() throws UpdateException, RestApiException {
try {
return super.call();
- } catch (Throwable t) {
- Throwables.throwIfUnchecked(t);
- Throwables.throwIfInstanceOf(t, UpdateException.class);
- Throwables.throwIfInstanceOf(t, RestApiException.class);
- throw new UpdateException(t);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, UpdateException.class);
+ Throwables.throwIfInstanceOf(e, RestApiException.class);
+ throw new UpdateException(e);
}
}
}
diff --git a/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
index cf733a6..d66edcf 100644
--- a/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
+++ b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
@@ -87,9 +87,9 @@
public T call() {
try {
return super.call();
- } catch (Throwable t) {
- Throwables.throwIfUnchecked(t);
- throw new StorageException(t);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ throw new StorageException(e);
}
}
}
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java
index 48a5512..42aabfb 100644
--- a/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -370,7 +370,7 @@
err.flush();
} catch (IOException e2) {
// Ignored
- } catch (Throwable e2) {
+ } catch (RuntimeException e2) {
logger.atWarning().withCause(e2).log("Cannot send failure message to client");
}
return f.exitCode;
@@ -381,7 +381,7 @@
err.flush();
} catch (IOException e2) {
// Ignored
- } catch (Throwable e2) {
+ } catch (RuntimeException e2) {
logger.atWarning().withCause(e2).log("Cannot send internal server error message to client");
}
return 128;
@@ -500,15 +500,15 @@
out.flush();
err.flush();
- } catch (Throwable e) {
+ } catch (Exception e) {
try {
out.flush();
- } catch (Throwable e2) {
+ } catch (Exception e2) {
// Ignored
}
try {
err.flush();
- } catch (Throwable e2) {
+ } catch (Exception e2) {
// Ignored
}
rc = handleError(e);
diff --git a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
index 773c25b..d5f0ee8 100644
--- a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
+++ b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
@@ -138,7 +138,7 @@
// to do with the key object, and instead we must abort this load.
//
throw e;
- } catch (Throwable e) {
+ } catch (Exception e) {
markInvalid(k);
}
}
diff --git a/java/com/google/gerrit/sshd/commands/UploadArchive.java b/java/com/google/gerrit/sshd/commands/UploadArchive.java
index 0eda433..c1f4a7b 100644
--- a/java/com/google/gerrit/sshd/commands/UploadArchive.java
+++ b/java/com/google/gerrit/sshd/commands/UploadArchive.java
@@ -214,16 +214,16 @@
} catch (GitAPIException e) {
throw new Failure(7, "fatal: git api exception, " + e);
}
- } catch (Throwable t) {
+ } catch (Exception e) {
// Report the error in ERROR sideband channel. Catch Throwable too so we can also catch
// NoClassDefFound.
try (SideBandOutputStream sidebandError =
new SideBandOutputStream(
SideBandOutputStream.CH_ERROR, SideBandOutputStream.MAX_BUF, out)) {
- sidebandError.write(t.getMessage().getBytes(UTF_8));
+ sidebandError.write(e.getMessage().getBytes(UTF_8));
sidebandError.flush();
}
- throw t;
+ throw e;
} finally {
// In any case, cleanly close the packetOut channel
packetOut.end();
