Bug 558642: SafeRunner can return results

SafeRunner has a second run method which expects a
ISafeRunnableWithResult<T>. The run method returns this result.
Also some refactoring of the handleException method has been done (boy
scout rule).

Furthermore test have been improved:
- adapt to lambda
- better test method names to recognize what was wrong
- better text messages in assertions

Change-Id: I65c9104c3833fb6fe9044a2a3a73e477e0636e40
Signed-off-by: Marcus Hoepfner <marcus.hoepfner@sap.com>

5 files changed, 190 insertions, 106 deletions

diff --git a/bundles/org.eclipse.equinox.common.tests/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.common.tests/META-INF/MANIFEST.MF
index de575e4fd..afab99b89 100644
--- a/bundles/org.eclipse.equinox.common.tests/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.equinox.common.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Common Eclipse Runtime Tests
 Bundle-SymbolicName: org.eclipse.equinox.common.tests;singleton:=true
-Bundle-Version: 3.10.600.qualifier
+Bundle-Version: 3.11.0.qualifier
 Automatic-Module-Name: org.eclipse.equinox.common.tests
 Bundle-RequiredExecutionEnvironment: JavaSE-1.8
 Bundle-ActivationPolicy: lazy
diff --git a/bundles/org.eclipse.equinox.common.tests/pom.xml b/bundles/org.eclipse.equinox.common.tests/pom.xml
index 3b24e742e..6e5f5e3ef 100644
--- a/bundles/org.eclipse.equinox.common.tests/pom.xml
+++ b/bundles/org.eclipse.equinox.common.tests/pom.xml
@@ -19,6 +19,6 @@
   </parent>
   <groupId>org.eclipse.equinox</groupId>
   <artifactId>org.eclipse.equinox.common.tests</artifactId>
-  <version>3.10.600-SNAPSHOT</version>
+  <version>3.11.0-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
 </project>
diff --git a/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/SafeRunnerTest.java b/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/SafeRunnerTest.java
index 7edf55d0a..c209ab8a9 100644
--- a/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/SafeRunnerTest.java
+++ b/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/SafeRunnerTest.java
@@ -13,7 +13,10 @@
  *******************************************************************************/
 package org.eclipse.equinox.common.tests;
 
-import org.eclipse.core.runtime.*;
+import org.eclipse.core.runtime.ISafeRunnable;
+import org.eclipse.core.runtime.ISafeRunnableWithResult;
+import org.eclipse.core.runtime.OperationCanceledException;
+import org.eclipse.core.runtime.SafeRunner;
 import org.eclipse.core.tests.harness.CoreTest;
 
 /**
@@ -22,89 +25,36 @@ import org.eclipse.core.tests.harness.CoreTest;
 public class SafeRunnerTest extends CoreTest {
 
 	/**
-	 * Ensures that cancelation exceptions are handled
+	 * Ensures that cancellation exceptions are handled
 	 */
-	public void testCancel() {
-		boolean caught = false;
+	public void testOperationCanceledExceptionAreHandled() {
 		try {
-			SafeRunner.run(new ISafeRunnable() {
-				@Override
-				public void handleException(Throwable exception) {
-				}
-
-				@Override
-				public void run() throws Exception {
-					throw new OperationCanceledException();
-				}
+			SafeRunner.run(() -> {
+				throw new OperationCanceledException();
 			});
 		} catch (OperationCanceledException e) {
-			caught = true;
+			fail("OperationCanceledException unexpectedly caught.", e);
 		}
-		assertFalse("1.0", caught);
 	}
 
-	/**
-	 * Tests that SafeRunner catches the expected exception types.
-	 */
-	public void testCaughtExceptionTypes() {
-		Throwable[] failures = new Throwable[] {new AssertionError(), new LinkageError(), new RuntimeException()};
-		for (int i = 0; i < failures.length; i++) {
-			final Throwable[] handled = new Throwable[1];
-			final Throwable current = failures[i];
-			try {
-				SafeRunner.run(new ISafeRunnable() {
-					@Override
-					public void handleException(Throwable exception) {
-						handled[0] = exception;
-					}
+	public void testAssertionErrorIsCaught() {
+		assertExceptionHandled(new AssertionError());
+	}
 
-					@Override
-					public void run() throws Exception {
-						if (current instanceof Exception) {
-							throw (Exception) current;
-						} else if (current instanceof Error) {
-							throw (Error) current;
-						}
-					}
-				});
-			} catch (Throwable t) {
-				fail("1." + i, t);
-			}
-			assertEquals("2." + i, current, handled[0]);
-		}
+	public void testLinkageErrorIsCaught() {
+		assertExceptionHandled(new LinkageError());
+	}
 
+	public void testRuntimeExceptionIsCaught() {
+		assertExceptionHandled(new RuntimeException());
 	}
 
-	/**
-	 * Tests that SafeRunner re-throws expected exception types.
-	 */
-	public void testThrownExceptionTypes() {
-		//thrown exceptions
-		final Throwable[] thrown = new Throwable[] {new Error(), new OutOfMemoryError()};
-		for (int i = 0; i < thrown.length; i++) {
-			boolean caught = false;
-			final Throwable current = thrown[i];
-			try {
-				SafeRunner.run(new ISafeRunnable() {
-					@Override
-					public void handleException(Throwable exception) {
-					}
+	public void testRethrowsError() {
+		assertExceptionRethrown(new Error());
+	}
 
-					@Override
-					public void run() throws Exception {
-						if (current instanceof Exception) {
-							throw (Exception) current;
-						} else if (current instanceof Error) {
-							throw (Error) current;
-						}
-					}
-				});
-			} catch (Throwable t) {
-				assertEquals("1." + i, current, t);
-				caught = true;
-			}
-			assertTrue("2." + i, caught);
-		}
+	public void testRethrowsOutOfMemoryError() {
+		assertExceptionRethrown(new OutOfMemoryError());
 	}
 
 	public void testNull() {
@@ -112,7 +62,7 @@ public class SafeRunnerTest extends CoreTest {
 			SafeRunner.run(null);
 			fail("1.0");
 		} catch (RuntimeException e) {
-			//expected
+			// expected
 		}
 	}
 
@@ -120,7 +70,7 @@ public class SafeRunnerTest extends CoreTest {
 	 * Ensures that exceptions are propagated when the safe runner re-throws it
 	 */
 	public void testRethrow() {
-		boolean caught = false;
+		IllegalArgumentException caughtException = null;
 		try {
 			SafeRunner.run(new ISafeRunnable() {
 				@Override
@@ -136,9 +86,64 @@ public class SafeRunnerTest extends CoreTest {
 				}
 			});
 		} catch (IllegalArgumentException e) {
-			caught = true;
+			caughtException = e;
+		}
+		assertNotNull("Cathed exception expected.", caughtException);
+
+	}
+
+	public void testWithResult() {
+		assertEquals("TestRun", SafeRunner.run(() -> "TestRun"));
+	}
+
+	public void testWithResultReturnsNullOnException() {
+		ISafeRunnableWithResult<String> code = () -> {
+			throw new IllegalArgumentException();
+		};
+		assertNull(SafeRunner.run(code));
+	}
+
+	private void assertExceptionRethrown(Throwable current) {
+		Throwable caughtException = null;
+		try {
+			SafeRunner.run(new ISafeRunnable() {
+
+				@Override
+				public void run() throws Exception {
+					if (current instanceof Exception) {
+						throw (Exception) current;
+					} else if (current instanceof Error) {
+						throw (Error) current;
+					}
+				}
+			});
+		} catch (Throwable t) {
+			caughtException = t;
 		}
-		assertTrue("1.0", caught);
+		assertEquals("Unexpected exception.", current, caughtException);
+	}
 
+	private void assertExceptionHandled(Throwable throwable) {
+		final Throwable[] handled = new Throwable[1];
+		try {
+			SafeRunner.run(new ISafeRunnable() {
+				@Override
+				public void handleException(Throwable exception) {
+					handled[0] = exception;
+				}
+
+				@Override
+				public void run() throws Exception {
+					if (throwable instanceof Exception) {
+						throw (Exception) throwable;
+					} else if (throwable instanceof Error) {
+						throw (Error) throwable;
+					}
+				}
+			});
+		} catch (Throwable t) {
+			fail("Exception unexpectedly caught.", t);
+		}
+		assertEquals("Unexpected exception.", throwable, handled[0]);
 	}
-}
+}
\ No newline at end of file
diff --git a/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ISafeRunnableWithResult.java b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ISafeRunnableWithResult.java
new file mode 100644
index 000000000..4f98a942c
--- /dev/null
+++ b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ISafeRunnableWithResult.java
@@ -0,0 +1,35 @@
+package org.eclipse.core.runtime;
+
+/**
+ * Safe runnables represent blocks of code and associated exception
+ * handlers.  They are typically used when a plug-in needs to call some
+ * untrusted code (e.g., code contributed by another plug-in via an
+ * extension). In contradiction to {@link ISafeRunnable} this runnable is able to return a result.
+ * <p>
+ * This interface can be used without OSGi running.
+ * </p><p>
+ * Clients may implement this interface.
+ * </p>
+ * @param <T> the type of the result
+ * @see SafeRunner#run(ISafeRunnableWithResult)
+ * 
+ * @since 3.11
+ */
+@FunctionalInterface
+public interface ISafeRunnableWithResult<T> extends ISafeRunnable {
+	@Override
+	default void run() throws Exception {
+		runWithResult();
+	}
+
+	/**
+	 * Runs this runnable and returns the result. Any exceptions thrown from this method will
+	 * be logged by the caller and passed to this runnable's
+	 * {@link #handleException} method.
+	 * @return the result
+	 *
+	 * @exception Exception if a problem occurred while running this method
+	 * @see SafeRunner#run(ISafeRunnable)
+	 */
+	public T runWithResult() throws Exception;
+}
\ No newline at end of file
diff --git a/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/SafeRunner.java b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/SafeRunner.java
index 3d6861f8b..3a0870115 100644
--- a/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/SafeRunner.java
+++ b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/SafeRunner.java
@@ -28,9 +28,9 @@ import org.eclipse.osgi.util.NLS;
 public final class SafeRunner {
 
 	/**
-	 * Runs the given runnable in a protected mode.   Exceptions
+	 * Runs the given runnable in a protected mode. Exceptions
 	 * thrown in the runnable are logged and passed to the runnable's
-	 * exception handler.  Such exceptions are not rethrown by this method.
+	 * exception handler. Such exceptions are not rethrown by this method.
 	 * <p>
 	 * In addition to catching all {@link Exception} types, this method also catches certain {@link Error} 
 	 * types that typically result from programming errors in the code being executed. 
@@ -40,37 +40,81 @@ public final class SafeRunner {
 	 * @param code the runnable to run
 	 */
 	public static void run(ISafeRunnable code) {
+		run(new ISafeRunnableWithResult<Void>() {
+
+			@Override
+			public Void runWithResult() throws Exception {
+				code.run();
+				return null;
+			}
+
+			@Override
+			public void handleException(Throwable exception) {
+				code.handleException(exception);
+			}
+		});
+	}
+
+	/**
+	 * Runs the given runnable in a protected mode and returns the result given by the runnable. Exceptions
+	 * thrown in the runnable are logged and passed to the runnable's
+	 * exception handler. Such exceptions are not rethrown by this method, instead null is returned.
+	 * <p>
+	 * In addition to catching all {@link Exception} types, this method also catches certain {@link Error}
+	 * types that typically result from programming errors in the code being executed. 
+	 * Severe errors that are not generally safe to catch are not caught by this method.
+	 * </p>
+	 * @param <T> the result type
+	 *
+	 * @param code the runnable to run
+	 * @return the result
+	 *
+	 * @since 3.11
+	 */
+	public static <T> T run(ISafeRunnableWithResult<T> code) {
 		Assert.isNotNull(code);
 		try {
-			code.run();
+			return code.runWithResult();
 		} catch (Exception | LinkageError | AssertionError e) {
 			handleException(code, e);
+			return null;
 		}
 	}
 
-	private static void handleException(ISafeRunnable code, Throwable e) {
-		if (!(e instanceof OperationCanceledException)) {
-			// try to obtain the correct plug-in id for the bundle providing the safe runnable 
-			Activator activator = Activator.getDefault();
-			String pluginId = null;
-			if (activator != null)
-				pluginId = activator.getBundleId(code);
-			if (pluginId == null)
-				pluginId = IRuntimeConstants.PI_COMMON;
-			String message = NLS.bind(CommonMessages.meta_pluginProblems, pluginId);
-			IStatus status;
-			if (e instanceof CoreException) {
-				status = new MultiStatus(pluginId, IRuntimeConstants.PLUGIN_ERROR, message, e);
-				((MultiStatus) status).merge(((CoreException) e).getStatus());
-			} else {
-				status = new Status(IStatus.ERROR, pluginId, IRuntimeConstants.PLUGIN_ERROR, message, e);
-			}
-			// Make sure user sees the exception: if the log is empty, log the exceptions on stderr 
-			if (!RuntimeLog.isEmpty())
-				RuntimeLog.log(status);
-			else
-				e.printStackTrace();
+	private static void handleException(ISafeRunnable code, Throwable exception) {
+		if (!(exception instanceof OperationCanceledException)) {
+			String pluginId = getBundleIdOfSafeRunnable(code);
+			IStatus status = convertToStatus(exception, pluginId);
+			makeSureUserSeesException(exception, status);
+		}
+		code.handleException(exception);
+	}
+
+	private static void makeSureUserSeesException(Throwable exception, IStatus status) {
+		if (RuntimeLog.isEmpty()) {
+			exception.printStackTrace();
+		} else {
+			RuntimeLog.log(status);
+		}
+	}
+
+	private static String getBundleIdOfSafeRunnable(ISafeRunnable code) {
+		Activator activator = Activator.getDefault();
+		String pluginId = null;
+		if (activator != null)
+			pluginId = activator.getBundleId(code);
+		if (pluginId == null)
+			return IRuntimeConstants.PI_COMMON;
+		return pluginId;
+	}
+
+	private static IStatus convertToStatus(Throwable exception, String pluginId) {
+		String message = NLS.bind(CommonMessages.meta_pluginProblems, pluginId);
+		if (exception instanceof CoreException) {
+			MultiStatus status = new MultiStatus(pluginId, IRuntimeConstants.PLUGIN_ERROR, message, exception);
+			status.merge(((CoreException) exception).getStatus());
+			return status;
 		}
-		code.handleException(e);
+		return new Status(IStatus.ERROR, pluginId, IRuntimeConstants.PLUGIN_ERROR, message, exception);
 	}
-}
+}
\ No newline at end of file