From 9e0dfe9c24bcbf58ce2ab8574fa80ece458db438 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Wed, 12 Aug 2015 21:22:52 +0100 Subject: [PATCH] Fix DefaultStateMachineExecutor concurrency issue - Check task is null and then set it was wrongly not made as thread safe, changing it to be wrapped inside AtomicReference. - This relates to #76 and hopefully fixes #96 --- .../support/DefaultStateMachineExecutor.java | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/DefaultStateMachineExecutor.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/DefaultStateMachineExecutor.java index 0ca8b188..064a036c 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/DefaultStateMachineExecutor.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/DefaultStateMachineExecutor.java @@ -27,6 +27,7 @@ import java.util.Map.Entry; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -83,12 +84,12 @@ public class DefaultStateMachineExecutor extends LifecycleObjectSupport im private final AtomicBoolean initialHandled = new AtomicBoolean(false); - private volatile Runnable task; + private final AtomicReference taskRef = new AtomicReference(); private StateMachineExecutorTransit stateMachineExecutorTransit; - private final StateMachineInterceptorList interceptors = - new StateMachineInterceptorList(); + private final StateMachineInterceptorList interceptors = + new StateMachineInterceptorList(); /** * Instantiates a new default state machine executor. @@ -205,21 +206,26 @@ public class DefaultStateMachineExecutor extends LifecycleObjectSupport im if (executor == null) { return; } - if (task == null) { - task = new Runnable() { - @Override - public void run() { - processEventQueue(); + + // TODO: it'd be nice not to create runnable if + // current ref is null, we use atomic reference + // to play safe with concurrency + Runnable task = new Runnable() { + @Override + public void run() { + processEventQueue(); + processTriggerQueue(); + while (processDeferList()) { processTriggerQueue(); - while (processDeferList()) { - processTriggerQueue(); - } - task = null; - if (requestTask.getAndSet(false)) { - scheduleEventQueueProcessing(); - } } - }; + taskRef.set(null); + if (requestTask.getAndSet(false)) { + scheduleEventQueueProcessing(); + } + } + }; + + if (taskRef.compareAndSet(null, task)) { executor.execute(task); } else { requestTask.set(true);