Guard should not break with Throwable

- Now catching all exceptions and throwables
  with evaluate and return false in those cases.
- Fixes #206

Author: jvalkeal

spring-statemachine-core/src/main/java/org/springframework/statemachine/state/ChoicePseudoState.java

@@ -17,6 +17,8 @@
 
 import java.util.List;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.statemachine.StateContext;
 import org.springframework.statemachine.guard.Guard;
 import org.springframework.util.Assert;
@@ -31,6 +33,7 @@
  */
 public class ChoicePseudoState<S, E> implements PseudoState<S, E> {
 
+	private final static Log log = LogFactory.getLog(ChoicePseudoState.class);
 	private final List<ChoiceStateData<S, E>> choices;
 
 	/**
@@ -52,7 +55,7 @@ public State<S, E> entry(StateContext<S, E> context) {
 		State<S, E> s = null;
 		for (ChoiceStateData<S, E> c : choices) {
 			s = c.getState();
-			if (c.guard != null && c.guard.evaluate(context)) {
+			if (c.guard != null && evaluateInternal(c.guard, context)) {
 				break;
 			}
 		}
@@ -67,6 +70,15 @@ public void exit(StateContext<S, E> context) {
 	public void addPseudoStateListener(PseudoStateListener<S, E> listener) {
 	}
 
+	private boolean evaluateInternal(Guard<S, E> guard, StateContext<S, E> context) {
+		try {
+			return guard.evaluate(context);
+		} catch (Throwable t) {
+			log.warn("Deny guard due to throw as GUARD should not error", t);
+			return false;
+		}
+	}
+
 	/**
 	 * Data class wrapping choice {@link State} and {@link Guard}
 	 * together.

spring-statemachine-core/src/main/java/org/springframework/statemachine/state/JunctionPseudoState.java

@@ -17,6 +17,8 @@
 
 import java.util.List;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.statemachine.StateContext;
 import org.springframework.statemachine.guard.Guard;
 import org.springframework.util.Assert;
@@ -31,6 +33,7 @@
  */
 public class JunctionPseudoState<S, E> implements PseudoState<S, E> {
 
+	private final static Log log = LogFactory.getLog(JunctionPseudoState.class);
 	private final List<JunctionStateData<S, E>> junctions;
 
 	/**
@@ -52,7 +55,7 @@ public State<S, E> entry(StateContext<S, E> context) {
 		State<S, E> s = null;
 		for (JunctionStateData<S, E> j : junctions) {
 			s = j.getState();
-			if (j.guard != null && j.guard.evaluate(context)) {
+			if (j.guard != null && evaluateInternal(j.guard, context)) {
 				break;
 			}
 		}
@@ -67,6 +70,15 @@ public void exit(StateContext<S, E> context) {
 	public void addPseudoStateListener(PseudoStateListener<S, E> listener) {
 	}
 
+	private boolean evaluateInternal(Guard<S, E> guard, StateContext<S, E> context) {
+		try {
+			return guard.evaluate(context);
+		} catch (Throwable t) {
+			log.warn("Deny guard due to throw as GUARD should not error", t);
+			return false;
+		}
+	}
+
 	/**
 	 * Data class wrapping choice {@link State} and {@link Guard}
 	 * together.

spring-statemachine-core/src/main/java/org/springframework/statemachine/transition/AbstractTransition.java

@@ -17,6 +17,8 @@
 
 import java.util.Collection;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.statemachine.StateContext;
 import org.springframework.statemachine.action.Action;
 import org.springframework.statemachine.guard.Guard;
@@ -35,6 +37,8 @@
  */
 public abstract class AbstractTransition<S, E> implements Transition<S, E> {
 
+	private final static Log log = LogFactory.getLog(AbstractTransition.class);
+
 	private final State<S,E> source;
 
 	private final State<S,E> target;
@@ -90,7 +94,12 @@ public Trigger<S, E> getTrigger() {
 	@Override
 	public boolean transit(StateContext<S, E> context) {
 		if (guard != null) {
-			if (!guard.evaluate(context)) {
+			try {
+				if (!guard.evaluate(context)) {
+					return false;
+				}
+			} catch (Throwable t) {
+				log.warn("Deny guard due to throw as GUARD should not error", t);
 				return false;
 			}
 		}

spring-statemachine-core/src/test/java/org/springframework/statemachine/guard/GuardTests.java

@@ -33,6 +33,7 @@
 import org.springframework.statemachine.AbstractStateMachineTests.TestGuard;
 import org.springframework.statemachine.AbstractStateMachineTests.TestStates;
 import org.springframework.statemachine.ObjectStateMachine;
+import org.springframework.statemachine.StateContext;
 import org.springframework.statemachine.StateMachineSystemConstants;
 import org.springframework.statemachine.config.EnableStateMachine;
 import org.springframework.statemachine.config.EnumStateMachineConfigurerAdapter;
@@ -41,7 +42,7 @@
 
 /**
  * Tests for state machine guards.
- * 
+ *
  * @author Janne Valkealahti
  *
  */
@@ -57,12 +58,12 @@ public void testGuardEvaluated() throws Exception {
 		TestAction testAction = ctx.getBean("testAction", TestAction.class);
 		assertThat(testGuard, notNullValue());
 		assertThat(testAction, notNullValue());
-		
+
 		machine.start();
-		machine.sendEvent(TestEvents.E1);		
+		machine.sendEvent(TestEvents.E1);
 		assertThat(testGuard.onEvaluateLatch.await(2, TimeUnit.SECONDS), is(true));
 		assertThat(testAction.onExecuteLatch.await(2, TimeUnit.SECONDS), is(true));
-		
+
 		ctx.close();
 	}
 
@@ -76,18 +77,35 @@ public void testGuardDenyAction() throws Exception {
 		TestAction testAction = ctx.getBean("testAction", TestAction.class);
 		assertThat(testGuard, notNullValue());
 		assertThat(testAction, notNullValue());
-		
+
 		machine.start();
 		assertThat(machine.getState().getIds(), contains(TestStates.S1));
-		
-		machine.sendEvent(TestEvents.E1);		
+
+		machine.sendEvent(TestEvents.E1);
 		assertThat(testGuard.onEvaluateLatch.await(2, TimeUnit.SECONDS), is(true));
 		assertThat(testAction.onExecuteLatch.await(2, TimeUnit.SECONDS), is(false));
 		assertThat(machine.getState().getIds(), contains(TestStates.S1));
-		
+
 		ctx.close();
 	}
-	
+
+	@SuppressWarnings({ "unchecked" })
+	@Test
+	public void testGuardThrows() throws Exception {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Config3.class);
+		ObjectStateMachine<TestStates,TestEvents> machine =
+				ctx.getBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE, ObjectStateMachine.class);
+		machine.start();
+		assertThat(machine.getState().getIds(), contains(TestStates.S1));
+		machine.sendEvent(TestEvents.E1);
+		assertThat(machine.getState().getIds(), contains(TestStates.S1));
+		machine.sendEvent(TestEvents.E2);
+		assertThat(machine.getState().getIds(), contains(TestStates.S1));
+		machine.sendEvent(TestEvents.E3);
+		assertThat(machine.getState().getIds(), contains(TestStates.S2));
+		ctx.close();
+	}
+
 	@Configuration
 	@EnableStateMachine
 	public static class Config1 extends EnumStateMachineConfigurerAdapter<TestStates, TestEvents> {
@@ -116,12 +134,12 @@ public void configure(StateMachineTransitionConfigurer<TestStates, TestEvents> t
 		public TestAction testAction() {
 			return new TestAction();
 		}
-		
+
 		@Bean
 		public TestGuard testGuard() {
 			return new TestGuard(true);
 		}
-		
+
 		@Bean
 		public TaskExecutor taskExecutor() {
 			return new SyncTaskExecutor();
@@ -152,12 +170,12 @@ public void configure(StateMachineTransitionConfigurer<TestStates, TestEvents> t
 					.action(testAction())
 					.guard(testGuard());
 		}
-		
+
 		@Bean
 		public TestGuard testGuard() {
 			return new TestGuard(false);
 		}
-		
+
 		@Bean
 		public TestAction testAction() {
 			return new TestAction();
@@ -169,5 +187,61 @@ public TaskExecutor taskExecutor() {
 		}
 
 	}
-	
+
+	@Configuration
+	@EnableStateMachine
+	public static class Config3 extends EnumStateMachineConfigurerAdapter<TestStates, TestEvents> {
+
+		@Override
+		public void configure(StateMachineStateConfigurer<TestStates, TestEvents> states) throws Exception {
+			states
+				.withStates()
+					.initial(TestStates.S1)
+					.state(TestStates.S1)
+					.state(TestStates.S2);
+		}
+
+		@Override
+		public void configure(StateMachineTransitionConfigurer<TestStates, TestEvents> transitions) throws Exception {
+			transitions
+				.withExternal()
+					.source(TestStates.S1)
+					.target(TestStates.S2)
+					.event(TestEvents.E1)
+					.guard(testGuard1())
+					.and()
+				.withExternal()
+					.source(TestStates.S1)
+					.target(TestStates.S2)
+					.event(TestEvents.E2)
+					.guard(testGuard2())
+					.and()
+				.withExternal()
+					.source(TestStates.S1)
+					.target(TestStates.S2)
+					.event(TestEvents.E3);
+		}
+
+		@Bean
+		public Guard<TestStates, TestEvents> testGuard1() {
+			return new Guard<TestStates, TestEvents>() {
+
+				@Override
+				public boolean evaluate(StateContext<TestStates, TestEvents> context) {
+					throw new RuntimeException();
+				}
+			};
+		}
+
+		@Bean
+		public Guard<TestStates, TestEvents> testGuard2() {
+			return new Guard<TestStates, TestEvents>() {
+
+				@Override
+				public boolean evaluate(StateContext<TestStates, TestEvents> context) {
+					throw new Error();
+				}
+			};
+		}
+	}
 }