From e15580c310e420c78ca7023018ae22aaf6f2c1cb Mon Sep 17 00:00:00 2001 From: Giles Knap Date: Mon, 14 Nov 2016 10:16:06 +0000 Subject: [PATCH] clean up thread pools and fix initial_reset race condition --- malcolm/controllers/runnablecontroller.py | 4 ++-- malcolm/core/process.py | 2 ++ malcolm/core/statemachine.py | 2 +- malcolm/parts/builtin/runnablechildpart.py | 14 +++++++++----- .../test_websocket/test_system_websocket.py | 11 ++++++----- tests/test_controllers/test_managercontroller.py | 16 +++++++++------- .../test_controllers/test_runnablecontroller.py | 16 ++++++++-------- tests/test_core/test_statemachine.py | 2 +- 8 files changed, 38 insertions(+), 29 deletions(-) diff --git a/malcolm/controllers/runnablecontroller.py b/malcolm/controllers/runnablecontroller.py index 04cbc135c..f5ccb00fe 100644 --- a/malcolm/controllers/runnablecontroller.py +++ b/malcolm/controllers/runnablecontroller.py @@ -324,8 +324,8 @@ def update_completed_steps(self, completed_steps, part): self.completed_steps.set_value(min_completed_steps) @method_writeable_in( - sm.IDLE, sm.CONFIGURING, sm.READY, sm.RUNNING, sm.POSTRUN, sm.RESETTING, - sm.PAUSED, sm.SEEKING) + sm.IDLE, sm.CONFIGURING, sm.READY, sm.RUNNING, sm.POSTRUN, sm.PAUSED, + sm.SEEKING) def abort(self): self.try_stateful_function(sm.ABORTING, sm.ABORTED, self.do_abort) diff --git a/malcolm/core/process.py b/malcolm/core/process.py index 97bcd320a..d2bfc4f5f 100644 --- a/malcolm/core/process.py +++ b/malcolm/core/process.py @@ -96,6 +96,8 @@ def stop(self, timeout=None): # Now wait for anything it spawned to complete for s in self._other_spawned: s.wait(timeout=timeout) + # Garbage collect the syncfactory + del self.sync_factory def _forward_block_request(self, request): """Lookup target Block and spawn block.handle_request(request) diff --git a/malcolm/core/statemachine.py b/malcolm/core/statemachine.py index 0506a91b9..d726d8baa 100644 --- a/malcolm/core/statemachine.py +++ b/malcolm/core/statemachine.py @@ -153,7 +153,7 @@ def create_states(self): # Add Abort to all normal states normal_states = [ self.IDLE, self.READY, self.CONFIGURING, self.RUNNING, self.POSTRUN, - self.PAUSED, self.RESETTING, self.SEEKING] + self.PAUSED, self.SEEKING] for state in normal_states: self.set_allowed(state, self.ABORTING) diff --git a/malcolm/parts/builtin/runnablechildpart.py b/malcolm/parts/builtin/runnablechildpart.py index 6320d1cd7..85087b7a2 100644 --- a/malcolm/parts/builtin/runnablechildpart.py +++ b/malcolm/parts/builtin/runnablechildpart.py @@ -19,12 +19,16 @@ def update_configure_validate_args(self): @RunnableController.Reset def reset(self, task): # Wait until we are Idle - if self.child.state == sm.RESETTING: - task.when_matches(self.child["state"], sm.IDLE) - else: - if self.child["abort"].writeable: - task.post(self.child["abort"]) + if self.child["abort"].writeable: + task.post(self.child["abort"]) + try: task.post(self.child["reset"]) + except ValueError: + # We get a "ValueError: child is not writeable" if we can't run + # reset, probably because the child is already resetting, + # so just wait for it to be idle + task.when_matches( + self.child["state"], sm.IDLE, bad_values=[sm.FAULT]) self.update_configure_validate_args() @RunnableController.Validate diff --git a/tests/test_comms/test_websocket/test_system_websocket.py b/tests/test_comms/test_websocket/test_system_websocket.py index 8cf98120a..9146b0b39 100644 --- a/tests/test_comms/test_websocket/test_system_websocket.py +++ b/tests/test_comms/test_websocket/test_system_websocket.py @@ -28,8 +28,7 @@ class TestSystemWSCommsServerOnly(unittest.TestCase): socket = 8881 def setUp(self): - self.sf = SyncFactory("sync") - self.process = Process("proc", self.sf) + self.process = Process("proc", SyncFactory("sync")) Hello(self.process, dict(mri="hello")) self.process.add_comms( WebsocketServerComms(self.process, dict(port=self.socket))) @@ -69,8 +68,8 @@ class TestSystemWSCommsServerAndClient(unittest.TestCase): socket = 8882 def setUp(self): - self.sf = SyncFactory("sync") - self.process = Process("proc", self.sf) + sf = SyncFactory("sync") + self.process = Process("proc", sf) Hello(self.process, dict(mri="hello")) Counter(self.process, dict(mri="counter")) self.process.add_comms( @@ -79,7 +78,7 @@ def setUp(self): # If we don't wait long enough, sometimes the websocket_connect() # in process2 will hang... time.sleep(0.1) - self.process2 = Process("proc2", self.sf) + self.process2 = Process("proc2", sf) self.process2.add_comms( WebsocketClientComms(self.process2, dict(hostname="localhost", port=self.socket))) @@ -88,7 +87,9 @@ def setUp(self): def tearDown(self): self.socket += 1 self.process.stop() + del self.process self.process2.stop() + del self.process2 def test_server_hello_with_malcolm_client(self): block2 = self.process2.make_client_block("hello") diff --git a/tests/test_controllers/test_managercontroller.py b/tests/test_controllers/test_managercontroller.py index 030b2fb98..e0dd47c7b 100644 --- a/tests/test_controllers/test_managercontroller.py +++ b/tests/test_controllers/test_managercontroller.py @@ -14,7 +14,7 @@ # module imports from malcolm.controllers.managercontroller import ManagerController from malcolm.core import method_writeable_in, method_takes, DefaultStateMachine -from malcolm.core import Process, Part, Table +from malcolm.core import Process, Part, Table, Task from malcolm.core.syncfactory import SyncFactory from malcolm.parts.builtin.childpart import ChildPart @@ -28,8 +28,7 @@ def checkState(self, state, child=True, parent=True): self.assertEqual(self.c.state.value, state) def setUp(self): - self.s = SyncFactory('threading') - self.p = Process('process1', self.s) + self.p = Process('process1', SyncFactory('threading')) # create a child ManagerController block params = ManagerController.MethodMeta.\ @@ -56,12 +55,15 @@ def setUp(self): self.checkState(self.sm.DISABLED) self.p.start() - retry = 0 - while retry < 20 and self.c.state.value != self.sm.READY: - sleep(.1) - retry += 1 + # wait until block is Ready + task = Task("block_ready_task", self.p) + task.when_matches(self.b["state"], self.sm.READY, timeout=1) + self.checkState(self.sm.READY) + def tearDown(self): + self.p.stop() + def test_init(self): # the following block attributes should be created by a call to diff --git a/tests/test_controllers/test_runnablecontroller.py b/tests/test_controllers/test_runnablecontroller.py index 82a9e202d..31e737c10 100644 --- a/tests/test_controllers/test_runnablecontroller.py +++ b/tests/test_controllers/test_runnablecontroller.py @@ -15,7 +15,7 @@ from malcolm.core import method_writeable_in, method_takes, \ DefaultStateMachine, Block, Controller, REQUIRED from malcolm.core.vmetas import BooleanMeta -from malcolm.core import Process, Part, RunnableStateMachine +from malcolm.core import Process, Part, RunnableStateMachine, Task from malcolm.core.syncfactory import SyncFactory from malcolm.controllers.runnablecontroller import RunnableController from scanpointgenerator import LineGenerator, CompoundGenerator @@ -40,8 +40,7 @@ def checkSteps(self, configured, completed, total): def setUp(self): self.maxDiff = 5000 - self.s = SyncFactory('threading') - self.p = Process('process1', self.s) + self.p = Process('process1', SyncFactory('threading')) # create a child RunnableController block params = RunnableController.MethodMeta.prepare_input_map( @@ -68,13 +67,14 @@ def setUp(self): self.checkState(self.sm.DISABLED) self.p.start() - retry = 0 - while retry < 20 and self.c.state.value != self.sm.IDLE: - sleep(.5) - retry += 1 - self.checkState(self.sm.IDLE) + # wait until block is Ready + task = Task("block_ready_task", self.p) + task.when_matches(self.b["state"], self.sm.IDLE, timeout=1) + self.checkState(self.sm.IDLE) + def tearDown(self): + self.p.stop() def test_init(self): # the following block attributes should be created by a call to diff --git a/tests/test_core/test_statemachine.py b/tests/test_core/test_statemachine.py index 25fccb14e..73ec1e33c 100644 --- a/tests/test_core/test_statemachine.py +++ b/tests/test_core/test_statemachine.py @@ -106,7 +106,7 @@ def setUp(self): def test_init(self): default_allowed_transitions = OrderedDict() default_allowed_transitions['Resetting'] = { - "Idle", "Aborting", "Fault", "Disabling"} + "Idle", "Fault", "Disabling"} default_allowed_transitions['Idle'] = { "Configuring", "Aborting", 'Editing', "Fault", "Disabling"} default_allowed_transitions['Editing'] = {