From bbca0ea504ae0bc206c83e4eb40fc60d25065811 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Sat, 10 Jun 2017 09:11:35 +0100 Subject: [PATCH] Fix possible deadlock - Removing some sync blocks which imho are not needed anymore. - Adding some smoke tests. - Relates to #364 --- .../support/AbstractStateMachine.java | 37 +++----- .../statemachine/StateMachineSmokeTests.java | 95 +++++++++++++++++++ .../test/java/demo/persist/PersistTests.java | 21 +++- 3 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 spring-statemachine-core/src/test/java/org/springframework/statemachine/StateMachineSmokeTests.java diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java index 07ba7c08..9889f53f 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java @@ -115,9 +115,6 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo private volatile Message forwardedInitialEvent; - private final Object lock = new Object(); - private final Object lock2 = new Object(); - private StateMachine parentMachine; /** @@ -177,12 +174,10 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo public State getState() { // if we're complete assume we're stopped // and state was stashed into lastState - synchronized (lock) { - if (lastState != null && isComplete()) { - return lastState; - } else { - return currentState; - } + if (lastState != null && isComplete()) { + return lastState; + } else { + return currentState; } } @@ -212,9 +207,7 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo @Override public boolean sendEvent(Message event) { - synchronized (lock2) { - return sendEventInternal(event); - } + return sendEventInternal(event); } @Override @@ -381,15 +374,13 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo @Override protected void doStop() { - synchronized (lock) { - stateMachineExecutor.stop(); - notifyStateMachineStopped(buildStateContext(Stage.STATEMACHINE_STOP, null, null, this)); - // stash current state before we null it so that - // we can still return where we 'were' when machine is stopped - lastState = currentState; - currentState = null; - initialEnabled = null; - } + stateMachineExecutor.stop(); + notifyStateMachineStopped(buildStateContext(Stage.STATEMACHINE_STOP, null, null, this)); + // stash current state before we null it so that + // we can still return where we 'were' when machine is stopped + lastState = currentState; + currentState = null; + initialEnabled = null; } @Override @@ -938,9 +929,7 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo void setCurrentState(State state, Message message, Transition transition, boolean exit, StateMachine stateMachine, Collection> sources, Collection> targets) { - synchronized (lock2) { - setCurrentStateInternal(state, message, transition, exit, stateMachine, sources, targets); - } + setCurrentStateInternal(state, message, transition, exit, stateMachine, sources, targets); } private void setCurrentStateInternal(State state, Message message, Transition transition, boolean exit, diff --git a/spring-statemachine-core/src/test/java/org/springframework/statemachine/StateMachineSmokeTests.java b/spring-statemachine-core/src/test/java/org/springframework/statemachine/StateMachineSmokeTests.java new file mode 100644 index 00000000..80a02aa9 --- /dev/null +++ b/spring-statemachine-core/src/test/java/org/springframework/statemachine/StateMachineSmokeTests.java @@ -0,0 +1,95 @@ +/* + * Copyright 2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.statemachine; + +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.statemachine.config.EnableStateMachine; +import org.springframework.statemachine.config.EnumStateMachineConfigurerAdapter; +import org.springframework.statemachine.config.builders.StateMachineStateConfigurer; +import org.springframework.statemachine.config.builders.StateMachineTransitionConfigurer; + +public class StateMachineSmokeTests extends AbstractStateMachineTests { + + @Override + protected AnnotationConfigApplicationContext buildContext() { + return new AnnotationConfigApplicationContext(); + } + + @Test(timeout = 10000) + public void testLifecycleThreading() throws Exception { + context.register(Config1.class); + context.refresh(); + assertTrue(context.containsBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE)); + @SuppressWarnings("unchecked") + ObjectStateMachine machine = + context.getBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE, ObjectStateMachine.class); + assertThat(machine, notNullValue()); + + int cnt = 10; + Thread[] threads = new Thread[cnt]; + for (int i = 0; i < cnt; i++) { + threads[i] = new Thread(() -> { + for (int j = 0; j < 1000; j++) { + machine.start(); + machine.sendEvent(TestEvents.E1); + machine.stop(); + } + }); + } + for (Thread t : threads) { + t.start(); + } + for (Thread t : threads) { + t.join(); + } + assertThat(machine.getState(), nullValue()); + } + + @Configuration + @EnableStateMachine + static class Config1 extends EnumStateMachineConfigurerAdapter { + + @Override + public void configure(StateMachineStateConfigurer states) throws Exception { + states + .withStates() + .initial(TestStates.S1) + .state(TestStates.S2); + } + + @Override + public void configure(StateMachineTransitionConfigurer transitions) throws Exception { + transitions + .withExternal() + .source(TestStates.S1) + .target(TestStates.S2) + .event(TestEvents.E1) + .and() + .withExternal() + .source(TestStates.S2) + .target(TestStates.S1) + .event(TestEvents.E1); + } + } + +} diff --git a/spring-statemachine-samples/persist/src/test/java/demo/persist/PersistTests.java b/spring-statemachine-samples/persist/src/test/java/demo/persist/PersistTests.java index d57eef71..3817201f 100644 --- a/spring-statemachine-samples/persist/src/test/java/demo/persist/PersistTests.java +++ b/spring-statemachine-samples/persist/src/test/java/demo/persist/PersistTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2015 the original author or authors. + * Copyright 2015-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -89,6 +89,25 @@ public class PersistTests { assertThat(persist.listDbEntries(), containsString("id=2, state=SENT")); } + @Test + public void testThreadingSmoke() throws Exception { + int cnt = 10; + Thread[] threads = new Thread[cnt]; + for (int i = 0; i < cnt; i++) { + threads[i] = new Thread(() -> { + for (int j = 0; j < 1000; j++) { + persist.change(3, "SEND"); + } + }); + } + for (Thread t : threads) { + t.start(); + } + for (Thread t : threads) { + t.join(); + } + } + private static class TestListener extends StateMachineListenerAdapter { volatile CountDownLatch stateChangedLatch = new CountDownLatch(1);