Improve exception handling during startup
The main goal was to prevent StartupException from leaking to
Application.start() and Daemon, as was already prevented in main().
UnsupportedPlatformException no longer has a default message, as it is
convention for non-arg exception constructor to not have a message and
so having the message will likely cause confusion when it is misused
accidentally. Coupled with the fact that it wasn't documented what the
default was (and thus, could change at any point), nobody could actually
use it.
diff --git a/src/com/google/enterprise/adaptor/Application.java b/src/com/google/enterprise/adaptor/Application.java
index 49d96b0..5392432 100644
--- a/src/com/google/enterprise/adaptor/Application.java
+++ b/src/com/google/enterprise/adaptor/Application.java
@@ -89,8 +89,7 @@
* manual shutdown. A shutdown hook is automatically installed that calls
* {@code stop()}.
*/
- public synchronized void start() throws IOException, InterruptedException,
- StartupException {
+ public synchronized void start() throws IOException, InterruptedException {
synchronized (this) {
daemonInit();
@@ -100,7 +99,16 @@
Runtime.getRuntime().addShutdownHook(shutdownHook);
}
- daemonStart();
+ boolean success = false;
+ try {
+ daemonStart();
+ success = true;
+ } finally {
+ if (!success) {
+ // Call daemonDestroy() and remove shutdown hook.
+ stop(0, TimeUnit.SECONDS);
+ }
+ }
}
/**
@@ -112,21 +120,41 @@
if (primaryServer != null) {
throw new IllegalStateException("Already started");
}
- primaryServer = createHttpServer(config);
- dashboardServer = createDashboardHttpServer(config);
- // Because once stopped the server can't be reused, we can't reuse its
- // bind()ed socket if we stop it. So although ideally we would start/stop in
- // daemonStart/daemonStop, we instead must do it in
- // daemonInit/daemonDestroy.
- primaryServer.start();
- dashboardServer.start();
+ boolean success = false;
+ try {
+ primaryServer = createHttpServer(config);
+ dashboardServer = createDashboardHttpServer(config);
+ // Because once stopped the server can't be reused, we can't reuse its
+ // bind()ed socket if we stop it. So although ideally we would start/stop
+ // in daemonStart/daemonStop, we instead must do it in
+ // daemonInit/daemonDestroy.
+ primaryServer.start();
+ dashboardServer.start();
+ success = true;
+ } finally {
+ if (!success) {
+ daemonDestroy(0, TimeUnit.SECONDS);
+ }
+ }
}
/**
* Really start. This must be called after a successful {@link #daemonInit}.
*/
- synchronized void daemonStart() throws IOException, InterruptedException,
- StartupException {
+ synchronized void daemonStart() throws IOException, InterruptedException {
+ boolean success = false;
+ try {
+ realDaemonStart();
+ success = true;
+ } finally {
+ if (!success) {
+ daemonStop(0, TimeUnit.SECONDS);
+ }
+ }
+ }
+
+ private synchronized void realDaemonStart() throws IOException,
+ InterruptedException {
AdaptorContext context = gsa.setup(primaryServer, dashboardServer, null);
long sleepDurationMillis = 8000;
@@ -150,8 +178,7 @@
} catch (InterruptedException ex) {
throw ex;
} catch (StartupException ex) {
- log.log(Level.WARNING, "Failed to initialize adaptor", ex);
- throw ex;
+ throw new RuntimeException("Failed to initialize adaptor", ex);
} catch (Exception ex) {
log.log(Level.WARNING, "Failed to initialize adaptor", ex);
if (shutdownSemaphore.tryAcquire(sleepDurationMillis,
@@ -483,34 +510,13 @@
// Setup providing content.
try {
- try {
- app.start();
- log.info("doc content serving started");
- } catch (StartupException e) {
- throw new RuntimeException("could not start serving", e);
- } catch (IOException e) {
- throw new RuntimeException("could not start serving", e);
- }
+ app.start();
+ log.info("doc content serving started");
} catch (InterruptedException e) {
- // Do not call stop - it has already been called.
+ Thread.currentThread().interrupt();
throw new RuntimeException("could not start serving", e);
- } catch (Throwable t) {
- // Abnormal termination. Make sure any services that may have been
- // started are shut down.
- try {
- app.stop(3, TimeUnit.SECONDS);
- } catch (Throwable ignored) {
- // It was a noble try. Just get out.
- }
- // Now rethrow.
- if (t instanceof RuntimeException) {
- throw (RuntimeException) t;
- } else if (t instanceof Error) {
- throw (Error) t;
- } else {
- // Very unlikely to happen.
- throw new RuntimeException(t);
- }
+ } catch (IOException e) {
+ throw new RuntimeException("could not start serving", e);
}
return app;
}
diff --git a/src/com/google/enterprise/adaptor/GsaCommunicationHandler.java b/src/com/google/enterprise/adaptor/GsaCommunicationHandler.java
index f5168a7..dcd785e 100644
--- a/src/com/google/enterprise/adaptor/GsaCommunicationHandler.java
+++ b/src/com/google/enterprise/adaptor/GsaCommunicationHandler.java
@@ -57,7 +57,6 @@
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicInteger;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.net.ssl.SSLException;
@@ -121,12 +120,6 @@
private KeyPair keyPair;
private AclTransform aclTransform;
- /**
- * Used to stop startup prematurely. When greater than 0, start() should abort
- * immediately because stop() is currently processing. This allows cancelling
- * new start() calls before stop() is done processing.
- */
- private final AtomicInteger shutdownCount = new AtomicInteger();
private ShutdownWaiter waiter;
private final List<Filter> commonFilters = Arrays.asList(new Filter[] {
new AbortImmediatelyFilter(),
diff --git a/src/com/google/enterprise/adaptor/InvalidConfigurationException.java b/src/com/google/enterprise/adaptor/InvalidConfigurationException.java
index c66798c..bccdf3e 100644
--- a/src/com/google/enterprise/adaptor/InvalidConfigurationException.java
+++ b/src/com/google/enterprise/adaptor/InvalidConfigurationException.java
@@ -21,7 +21,7 @@
*/
public class InvalidConfigurationException extends StartupException {
/**
- * Constructs a new InvalidConfigurationException with no message and no root
+ * Constructs a new InvalidConfigurationException with no message and no
* cause.
*/
public InvalidConfigurationException() {
@@ -29,8 +29,8 @@
}
/**
- * Constructs a InvalidConfigurationException with a supplied message
- * but no root cause.
+ * Constructs a InvalidConfigurationException with a supplied message but no
+ * cause.
*
* @param message the message. Can be retrieved by the {@link #getMessage()}
* method.
@@ -40,13 +40,25 @@
}
/**
- * Constructs a InvalidConfigurationException with message and root cause.
+ * Constructs a InvalidConfigurationException with message and cause.
*
* @param message the message. Can be retrieved by the {@link #getMessage()}
* method.
- * @param rootCause root failure cause
+ * @param cause failure cause
*/
- public InvalidConfigurationException(String message, Throwable rootCause) {
- super(message, rootCause);
+ public InvalidConfigurationException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+ /**
+ * Constructs a InvalidConfigurationException with specified cause, copying
+ * its message if cause is non-{@code null}.
+ *
+ * @param message the message. Can be retrieved by the {@link #getMessage()}
+ * method.
+ * @param cause failure cause
+ */
+ public InvalidConfigurationException(Throwable cause) {
+ super(cause);
}
}
diff --git a/src/com/google/enterprise/adaptor/StartupException.java b/src/com/google/enterprise/adaptor/StartupException.java
index c8b5d3a..9464c46 100644
--- a/src/com/google/enterprise/adaptor/StartupException.java
+++ b/src/com/google/enterprise/adaptor/StartupException.java
@@ -17,21 +17,19 @@
/**
* Thrown for unrecoverable startup errors, such as fatal configuration
* errors or running on the wrong platform. StartupExceptions will bypass
- * the retry with back-off recovery logic and immediately terminate the
- * adaptor.
+ * the retry with back-off recovery logic of {@link Application} and immediately
+ * terminate the adaptor.
*/
public class StartupException extends RuntimeException {
/**
- * Constructs a new StartupException with no message and no root
- * cause.
+ * Constructs a new StartupException with no message and no cause.
*/
public StartupException() {
super();
}
/**
- * Constructs a StartupException with a supplied message but no root
- * cause.
+ * Constructs a StartupException with a supplied message but no cause.
*
* @param message the message. Can be retrieved by the {@link #getMessage()}
* method.
@@ -41,13 +39,23 @@
}
/**
- * Constructs a StartupException with message and root cause.
+ * Constructs a StartupException with message and cause.
*
* @param message the message. Can be retrieved by the {@link #getMessage()}
* method.
- * @param rootCause root failure cause
+ * @param cause failure cause
*/
- public StartupException(String message, Throwable rootCause) {
- super(message, rootCause);
+ public StartupException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+ /**
+ * Constructs a StartupException with specified cause, copying its message if
+ * cause is non-{@code null}.
+ *
+ * @param cause failure cause
+ */
+ public StartupException(Throwable cause) {
+ super(cause);
}
}
diff --git a/src/com/google/enterprise/adaptor/UnsupportedPlatformException.java b/src/com/google/enterprise/adaptor/UnsupportedPlatformException.java
index c004f36..3cd86d3 100644
--- a/src/com/google/enterprise/adaptor/UnsupportedPlatformException.java
+++ b/src/com/google/enterprise/adaptor/UnsupportedPlatformException.java
@@ -19,11 +19,10 @@
*/
public class UnsupportedPlatformException extends StartupException {
/**
- * Constructs a new UnsupportedPlatformException with a default message.
+ * Constructs a new UnsupportedPlatformException with no message.
*/
public UnsupportedPlatformException() {
- this(System.getProperty("os.name")
- + " is not a supported platform for this adaptor.");
+ super();
}
/**
@@ -35,4 +34,26 @@
public UnsupportedPlatformException(String message) {
super(message);
}
+
+ /**
+ * Constructs a UnsupportedPlatformException with a supplied message and
+ * cause.
+ *
+ * @param message the message. Can be retrieved by the {@link #getMessage()}
+ * method.
+ * @param cause failure cause
+ */
+ public UnsupportedPlatformException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+ /**
+ * Constructs a UnsupportedPlatformException with specified cause, copying its
+ * message if cause is non-{@code null}.
+ *
+ * @param cause failure cause
+ */
+ public UnsupportedPlatformException(Throwable cause) {
+ super(cause);
+ }
}
diff --git a/test/com/google/enterprise/adaptor/ApplicationTest.java b/test/com/google/enterprise/adaptor/ApplicationTest.java
index 12a9b8f..bce3236 100644
--- a/test/com/google/enterprise/adaptor/ApplicationTest.java
+++ b/test/com/google/enterprise/adaptor/ApplicationTest.java
@@ -276,10 +276,12 @@
@Test
public void testFailWithStartupException() throws Exception {
+ final StartupException startupException
+ = new StartupException("Unrecoverable error.");
class FailStartupAdaptor extends NullAdaptor {
@Override
public void init(AdaptorContext context) throws Exception {
- throw new StartupException("Unrecoverable error.");
+ throw startupException;
}
}
FailStartupAdaptor adaptor = new FailStartupAdaptor();
@@ -289,12 +291,13 @@
long startTime = System.nanoTime();
try {
app.start();
- fail("Expected a StartupException, but got none.");
- } catch (StartupException expected) {
+ fail("Expected a RuntimeException, but got none.");
+ } catch (RuntimeException expected) {
+ assertEquals(startupException, expected.getCause());
long duration = System.nanoTime() - startTime;
final long nanosInAMilli = 1000 * 1000;
if (duration > 1000 * nanosInAMilli) {
- fail("StartupException took a long time: " + duration);
+ fail("RuntimeException took a long time: " + duration);
}
}
}
