SCT now handles exceptions thrown by execution listeners

resolves #341

spring-cloud-task-core/src/main/java/org/springframework/cloud/task/listener/TaskLifecycleListener.java

@@ -17,6 +17,7 @@ package org.springframework.cloud.task.listener;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -31,6 +32,7 @@ import org.springframework.beans.factory.DisposableBean;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.ApplicationArguments;
 import org.springframework.boot.ExitCodeEvent;
+import org.springframework.boot.ExitCodeGenerator;
 import org.springframework.boot.context.event.ApplicationFailedEvent;
 import org.springframework.boot.context.event.ApplicationReadyEvent;
 import org.springframework.cloud.task.configuration.TaskProperties;
@@ -44,6 +46,7 @@ import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.context.SmartLifecycle;
 import org.springframework.context.event.ContextRefreshedEvent;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 
 /**
  * Monitors the lifecycle of a task.  This listener will record both the start and end of
@@ -63,6 +66,7 @@ import org.springframework.util.Assert;
  * property <code>spring.cloud.task.closecontext.enable</code> (defaults to true).
  *
  * @author Michael Minella
+ * @author Glenn Renfro
  */
 public class TaskLifecycleListener implements ApplicationListener<ApplicationEvent>, SmartLifecycle, DisposableBean {
 
@@ -86,6 +90,10 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 
 	private boolean finished = false;
 
+	private boolean listenerFailed = false;
+
+	private Throwable listenerException;
+
 	private TaskNameResolver taskNameResolver;
 
 	private ApplicationArguments applicationArguments;
@@ -148,34 +156,25 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 	}
 
 	private void doTaskEnd() {
-		if(this.started && !this.finished) {
+		if((this.listenerFailed || this.started) && !this.finished) {
 			this.taskExecution.setEndTime(new Date());
 
-			if(this.exitCodeEvent != null) {
-				this.taskExecution.setExitCode(exitCodeEvent.getExitCode());
-			}
-			else if(this.applicationFailedEvent != null){
-				this.taskExecution.setExitCode(1);
-			}
-			else{
-				this.taskExecution.setExitCode(0);
-			}
-
 			if(this.applicationFailedEvent != null) {
 				this.taskExecution.setErrorMessage(stackTraceToString(this.applicationFailedEvent.getException()));
 			}
 
-			if (this.applicationFailedEvent != null && this.taskExecution.getExitCode() != 0) {
-				taskExecution.setExitMessage(invokeOnTaskError(taskExecution,
-						this.applicationFailedEvent.getException()).getExitMessage());
+			this.taskExecution.setExitCode(calcExitStatus());
+			if (this.applicationFailedEvent != null) {
+				setExitMessage(invokeOnTaskError(this.taskExecution, this.applicationFailedEvent.getException()));
 			}
-			taskExecution.setExitMessage(invokeOnTaskEnd(taskExecution).getExitMessage());
-			taskRepository.completeTaskExecution(taskExecution.getExecutionId(), taskExecution.getExitCode(),
-					taskExecution.getEndTime(), taskExecution.getExitMessage(), taskExecution.getErrorMessage());
+
+			setExitMessage(invokeOnTaskEnd(this.taskExecution));
+			this.taskRepository.completeTaskExecution(this.taskExecution.getExecutionId(), this.taskExecution.getExitCode(),
+					this.taskExecution.getEndTime(), this.taskExecution.getExitMessage(), this.taskExecution.getErrorMessage());
 
 			this.finished = true;
 
-			if(taskProperties.getClosecontextEnabled() && this.context.isActive()) {
+			if(this.taskProperties.getClosecontextEnabled() && this.context.isActive()) {
 				this.context.close();
 			}
 
@@ -186,31 +185,66 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 		}
 	}
 
+	private void setExitMessage(TaskExecution taskExecutionParam) {
+		if(taskExecutionParam.getExitMessage() != null) {
+			this.taskExecution.setExitMessage(taskExecutionParam.getExitMessage());
+		}
+	}
+
+	private int calcExitStatus() {
+		int exitCode = 0;
+		if (this.exitCodeEvent != null) {
+			exitCode = this.exitCodeEvent.getExitCode();
+		}
+		else if (this.listenerFailed || this.applicationFailedEvent != null) {
+			Throwable exception = this.listenerException;
+			if (exception != null && exception instanceof TaskExecutionException) {
+				TaskExecutionException taskExecutionException = (TaskExecutionException) exception;
+				if (taskExecutionException.getCause() instanceof InvocationTargetException) {
+					InvocationTargetException invocationTargetException = (InvocationTargetException) taskExecutionException
+							.getCause();
+					if(invocationTargetException != null && invocationTargetException.getTargetException() != null) {
+						exception = invocationTargetException.getTargetException();
+					}
+				}
+			}
+
+			if (exception != null && exception instanceof ExitCodeGenerator) {
+				exitCode = ((ExitCodeGenerator) exception).getExitCode();
+			}
+			else {
+				exitCode = 1;
+			}
+		}
+
+		return exitCode;
+	}
+
 	private void doTaskStart() {
 
-		if(!started) {
+		if(!this.started) {
 			List<String> args = new ArrayList<>(0);
 
 			if(this.applicationArguments != null) {
 				args = Arrays.asList(this.applicationArguments.getSourceArgs());
 			}
-			if(taskProperties.getExecutionid() != null) {
-				TaskExecution taskExecution = taskExplorer.getTaskExecution(taskProperties.getExecutionid());
-				Assert.notNull(taskExecution, String.format("Invalid TaskExecution, ID %s not found", taskProperties.getExecutionid()));
+			if(this.taskProperties.getExecutionid() != null) {
+				TaskExecution taskExecution = this.taskExplorer.getTaskExecution(this.taskProperties.getExecutionid());
+				Assert.notNull(taskExecution, String.format("Invalid TaskExecution, ID %s not found", this.taskProperties.getExecutionid()));
 				Assert.isNull(taskExecution.getEndTime(), String.format(
-						"Invalid TaskExecution, ID %s task is already complete", taskProperties.getExecutionid()));
-				this.taskExecution = this.taskRepository.startTaskExecution(taskProperties.getExecutionid(),
+						"Invalid TaskExecution, ID %s task is already complete", this.taskProperties.getExecutionid()));
+				this.taskExecution = this.taskRepository.startTaskExecution(this.taskProperties.getExecutionid(),
 						this.taskNameResolver.getTaskName(), new Date(), args,
-						taskProperties.getExternalExecutionId(),
-						taskProperties.getParentExecutionId());
+						this.taskProperties.getExternalExecutionId(),
+						this.taskProperties.getParentExecutionId());
 			}
 			else {
 				TaskExecution taskExecution = new TaskExecution();
 				taskExecution.setTaskName(this.taskNameResolver.getTaskName());
 				taskExecution.setStartTime(new Date());
 				taskExecution.setArguments(args);
-				taskExecution.setExternalExecutionId(taskProperties.getExternalExecutionId());
-				taskExecution.setParentExecutionId(taskProperties.getParentExecutionId());
+				taskExecution.setExternalExecutionId(this.taskProperties.getExternalExecutionId());
+				taskExecution.setParentExecutionId(this.taskProperties.getParentExecutionId());
 				this.taskExecution = this.taskRepository.createTaskExecution(
 						taskExecution);
 			}
@@ -219,14 +253,23 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 			logger.error("Multiple start events have been received.  The first one was " +
 					"recorded.");
 		}
-		taskExecution.setExitMessage(invokeOnTaskStartup(taskExecution).getExitMessage());
+		setExitMessage(invokeOnTaskStartup(this.taskExecution));
 	}
 
 	private TaskExecution invokeOnTaskStartup(TaskExecution taskExecution){
 		TaskExecution listenerTaskExecution = getTaskExecutionCopy(taskExecution);
-		if (taskExecutionListeners != null) {
-			for (TaskExecutionListener taskExecutionListener : taskExecutionListeners) {
-				taskExecutionListener.onTaskStartup(listenerTaskExecution);
+		if (this.taskExecutionListeners != null) {
+			try {
+				for (TaskExecutionListener taskExecutionListener : this.taskExecutionListeners) {
+					taskExecutionListener.onTaskStartup(listenerTaskExecution);
+				}
+			}
+			catch (Throwable currentListenerException) {
+				logger.error(currentListenerException);
+				this.listenerFailed = true;
+				this.taskExecution.setErrorMessage(currentListenerException.getMessage());
+				this.listenerException = currentListenerException;
+				throw currentListenerException;
 			}
 		}
 		return listenerTaskExecution;
@@ -234,9 +277,20 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 
 	private TaskExecution invokeOnTaskEnd(TaskExecution taskExecution){
 		TaskExecution listenerTaskExecution = getTaskExecutionCopy(taskExecution);
-		if (taskExecutionListeners != null) {
-			for (TaskExecutionListener taskExecutionListener : taskExecutionListeners) {
-				taskExecutionListener.onTaskEnd(listenerTaskExecution);
+		if (this.taskExecutionListeners != null) {
+			try {
+				for (TaskExecutionListener taskExecutionListener : this.taskExecutionListeners) {
+					taskExecutionListener.onTaskEnd(listenerTaskExecution);
+				}
+			}
+			catch (Throwable listenerException) {
+				String errorMessage = stackTraceToString(listenerException);
+				if (StringUtils.hasText(listenerTaskExecution.getErrorMessage())) {
+					errorMessage = String.format("%s :Task also threw this Exception: %s", errorMessage, listenerTaskExecution.getErrorMessage());
+				}
+				logger.error(errorMessage);
+				listenerTaskExecution.setErrorMessage(errorMessage);
+				this.listenerFailed = true;
 			}
 		}
 		return listenerTaskExecution;
@@ -245,9 +299,27 @@ public class TaskLifecycleListener implements ApplicationListener<ApplicationEve
 	private TaskExecution invokeOnTaskError(TaskExecution taskExecution, Throwable throwable){
 		TaskExecution listenerTaskExecution = getTaskExecutionCopy(taskExecution);
 		if (taskExecutionListeners != null) {
-			for (TaskExecutionListener taskExecutionListener : taskExecutionListeners) {
-				taskExecutionListener.onTaskFailed(listenerTaskExecution, throwable);
+			try {
+				for (TaskExecutionListener taskExecutionListener : this.taskExecutionListeners) {
+					taskExecutionListener.onTaskFailed(listenerTaskExecution, throwable);
+				}
 			}
+			catch (Throwable listenerException) {
+				this.listenerFailed = true;
+				String errorMessage;
+				if(StringUtils.hasText(listenerTaskExecution.getErrorMessage())) {
+					errorMessage = String.format("%s :While handling " +
+							"this error: %s", listenerException.getMessage(),
+							listenerTaskExecution.getErrorMessage());
+				}
+				else {
+					errorMessage = listenerTaskExecution.getErrorMessage();
+				}
+				logger.error(errorMessage);
+				listenerTaskExecution.setErrorMessage(errorMessage);
+				listenerTaskExecution.setExitCode(1);
+			}
+
 		}
 		return listenerTaskExecution;
 	}

spring-cloud-task-core/src/main/java/org/springframework/cloud/task/listener/annotation/TaskListenerExecutor.java

@@ -87,8 +87,8 @@ public class TaskListenerExecutor implements TaskExecutionListener{
 				throw new TaskExecutionException("@BeforeTask and @AfterTask annotated methods must be public.", e);
 			}
 			catch (InvocationTargetException e) {
-				throw new TaskExecutionException("Failed to process @BeforeTask or @AfterTask" +
-						"annotation because: ", e);
+				throw new TaskExecutionException(String.format("Failed to process @BeforeTask or @AfterTask" +
+						" annotation because: %s", e.getTargetException().getMessage()), e);
 			}
 			catch (IllegalArgumentException e){
 				throw new TaskExecutionException("taskExecution parameter is required for @BeforeTask and @AfterTask annotated methods", e);
@@ -106,8 +106,8 @@ public class TaskListenerExecutor implements TaskExecutionListener{
 				throw new TaskExecutionException("@FailedTask annotated methods must be public.", e);
 			}
 			catch (InvocationTargetException e) {
-				throw new TaskExecutionException("Failed to process @FailedTask " +
-						"annotation because: ", e);
+				throw new TaskExecutionException(String.format("Failed to process @FailedTask " +
+						"annotation because: %s", e.getTargetException().getMessage()), e);
 			}
 			catch (IllegalArgumentException e){
 				throw new TaskExecutionException("taskExecution and throwable parameters "

spring-cloud-task-core/src/test/java/org/springframework/cloud/task/listener/TaskExecutionListenerTests.java

@@ -39,6 +39,7 @@ import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -54,6 +55,17 @@ public class TaskExecutionListenerTests {
 
 	private static final String EXCEPTION_MESSAGE = "This was expected";
 
+	public static boolean beforeTaskDidFireOnError = false;
+	public static boolean endTaskDidFireOnError = false;
+	public static boolean failedTaskDidFireOnError = false;
+
+	@BeforeTask
+	public void setup() {
+		beforeTaskDidFireOnError = false;
+		endTaskDidFireOnError = false;
+		failedTaskDidFireOnError = false;
+	}
+
 	@After
 	public void tearDown() {
 		if(context != null && context.isActive()) {
@@ -75,6 +87,58 @@ public class TaskExecutionListenerTests {
 		verifyListenerResults(true, false, false, taskExecution,taskExecutionListener);
 	}
 
+	/**
+	 * Verify that if a LifecycleProcessor executes all TaskExecutionListeners if BeforeTask throws exception.
+	 */
+	@Test
+	public void testBeforeTaskErrorCreate() {
+		boolean exceptionFired = false;
+		try {
+			setupContextForBeforeTaskErrorAnnotatedListener();
+		}
+		catch (Exception exception) {
+			exceptionFired = true;
+		}
+		assertTrue("Exception should have fired", exceptionFired);
+		assertTrue("BeforeTask Listener should have executed", beforeTaskDidFireOnError);
+		assertTrue("EndTask Listener should have executed", endTaskDidFireOnError);
+		assertFalse("FailedTask Listener should have executed", failedTaskDidFireOnError);
+	}
+
+	/**
+	 * Verify that if a LifecycleProcessor executes AfterTask TaskExecutionListeners if FailedTask throws exception.
+	 */
+	@Test
+	public void testFailedTaskErrorCreate() {
+		boolean exceptionFired = false;
+		try {
+			setupContextForFailedTaskErrorAnnotatedListener();
+		}
+		catch (Exception exception) {
+			exceptionFired = true;
+		}
+		assertTrue("Exception should have fired", exceptionFired);
+		assertTrue("EndTask Listener should have executed", endTaskDidFireOnError);
+		assertFalse("FailedTask Listener should not have executed", failedTaskDidFireOnError);
+	}
+
+	/**
+	 * Verify that if a LifecycleProcessor stores the correct exit code if AfterTask listener fails.
+	 */
+	@Test
+	public void testAfterTaskErrorCreate() {
+		setupContextForAfterTaskErrorAnnotatedListener();
+		AfterTaskErrorAnnotationConfiguration.AnnotatedTaskListener taskExecutionListener =
+				context.getBean(AfterTaskErrorAnnotationConfiguration.AnnotatedTaskListener.class);
+		context.publishEvent(new ApplicationReadyEvent(new SpringApplication(), new String[0], context));
+
+		assertEquals(true,taskExecutionListener.isTaskStartup());
+		assertEquals(true,taskExecutionListener.isTaskEnd());
+		assertEquals(TestListener.END_MESSAGE, taskExecutionListener.getTaskExecution().getExitMessage());
+		assertTrue(taskExecutionListener.getTaskExecution().getErrorMessage().contains("Failed to process @BeforeTask or @AfterTask annotation because: AfterTaskFailure"));
+		assertNull(taskExecutionListener.getThrowable());
+	}
+
 	/**
 	 * Verify that if a TaskExecutionListener Bean is present that the onTaskEnd method
 	 * is called.
@@ -173,7 +237,7 @@ public class TaskExecutionListenerTests {
 		}
 		else if(isTaskEnd){
 			assertEquals(TestListener.END_MESSAGE, actualListener.getTaskExecution().getExitMessage());
-			assertNull(actualListener.getTaskExecution().getErrorMessage());
+			assertEquals(taskExecution.getErrorMessage(), actualListener.getTaskExecution().getErrorMessage());
 			assertNull(actualListener.getThrowable());
 		}
 		else {
@@ -200,6 +264,24 @@ public class TaskExecutionListenerTests {
 		context.setId("annotatedTask");
 	}
 
+	private void setupContextForBeforeTaskErrorAnnotatedListener(){
+		context = new AnnotationConfigApplicationContext(TestDefaultConfiguration.class, BeforeTaskErrorAnnotationConfiguration.class,
+				PropertyPlaceholderAutoConfiguration.class);
+		context.setId("beforeTaskAnnotatedTask");
+	}
+
+	private void setupContextForFailedTaskErrorAnnotatedListener(){
+		context = new AnnotationConfigApplicationContext(TestDefaultConfiguration.class, FailedTaskErrorAnnotationConfiguration.class,
+				PropertyPlaceholderAutoConfiguration.class);
+		context.setId("failedTaskAnnotatedTask");
+	}
+
+	private void setupContextForAfterTaskErrorAnnotatedListener(){
+		context = new AnnotationConfigApplicationContext(TestDefaultConfiguration.class, AfterTaskErrorAnnotationConfiguration.class,
+				PropertyPlaceholderAutoConfiguration.class);
+		context.setId("afterTaskAnnotatedTask");
+	}
+
 	@Configuration
 	public static class DefaultAnnotationConfiguration {
 
@@ -241,6 +323,106 @@ public class TaskExecutionListenerTests {
 		}
 	}
 
+	@Configuration
+	public static class BeforeTaskErrorAnnotationConfiguration {
+
+		@Bean
+		public AnnotatedTaskListener annotatedTaskListener() {
+			return new AnnotatedTaskListener();
+		}
+
+		@Bean
+		public TaskListenerExecutorFactoryBean taskListenerExecutor(ConfigurableApplicationContext context) throws Exception
+		{
+			return new TaskListenerExecutorFactoryBean(context);
+		}
+
+		public static class AnnotatedTaskListener {
+
+			@BeforeTask
+			public void methodA(TaskExecution taskExecution) {
+				beforeTaskDidFireOnError = true;
+				throw new TaskExecutionException("BeforeTaskFailure");
+			}
+
+			@AfterTask
+			public void methodB(TaskExecution taskExecution) {
+				endTaskDidFireOnError = true;
+			}
+
+			@FailedTask
+			public void methodC(TaskExecution taskExecution, Throwable throwable) {
+				failedTaskDidFireOnError = true;
+			}
+		}
+	}
+
+	@Configuration
+	public static class FailedTaskErrorAnnotationConfiguration {
+
+		@Bean
+		public AnnotatedTaskListener annotatedTaskListener() {
+			return new AnnotatedTaskListener();
+		}
+
+		@Bean
+		public TaskListenerExecutorFactoryBean taskListenerExecutor(ConfigurableApplicationContext context) throws Exception
+		{
+			return new TaskListenerExecutorFactoryBean(context);
+		}
+
+		public static class AnnotatedTaskListener {
+
+			@BeforeTask
+			public void methodA(TaskExecution taskExecution) {
+				beforeTaskDidFireOnError = true;
+				throw new TaskExecutionException("BeforeTaskFailure");
+			}
+
+			@AfterTask
+			public void methodB(TaskExecution taskExecution) {
+				endTaskDidFireOnError = true;
+			}
+
+			@FailedTask
+			public void methodC(TaskExecution taskExecution, Throwable throwable) {
+				failedTaskDidFireOnError = true;
+				throw new TaskExecutionException("FailedTaskFailure");
+			}
+		}
+	}
+
+	@Configuration
+	public static class AfterTaskErrorAnnotationConfiguration {
+
+		@Bean
+		public AnnotatedTaskListener annotatedTaskListener() {
+			return new AnnotatedTaskListener();
+		}
+
+		@Bean
+		public TaskListenerExecutorFactoryBean taskListenerExecutor(ConfigurableApplicationContext context) throws Exception
+		{
+			return new TaskListenerExecutorFactoryBean(context);
+		}
+
+		public static class AnnotatedTaskListener extends TestListener{
+
+			@BeforeTask
+			public void methodA(TaskExecution taskExecution) {
+				isTaskStartup = true;
+			}
+
+			@AfterTask
+			public void methodB(TaskExecution taskExecution) {
+				isTaskEnd = true;
+				this.taskExecution = taskExecution;
+				this.taskExecution.setExitMessage(END_MESSAGE);
+				throw new TaskExecutionException("AfterTaskFailure");
+			}
+		}
+	}
+
 	@Configuration
 	public static class DefaultTaskListenerConfiguration {