Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wake executor when entities created or destroyed #336

Merged
merged 3 commits into from
May 6, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented May 2, 2019

Replaces #206
Fixes #188

This wakes the executor when entities are created or destroyed. I added tests for creating entities waking the executor, but I haven't figured out how to add tests for destroying entities waking the executor.

This was unblocked by #308 which made it possible to write a test for creating entities.

@sloretz sloretz added the in progress Actively being worked on (Kanban column) label May 2, 2019
@sloretz sloretz force-pushed the sloretz/wake_executor_on_create_or_destroy branch from 06e60b2 to 2b611a0 Compare May 2, 2019 01:40
@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 2, 2019
@sloretz
Copy link
Contributor Author

sloretz commented May 2, 2019

Proof that tests fail without 2b611a0
8: test/test_create_while_spinning.py FFFFF                                                                                                                                                                                                                                             [100%]
8: 
8: ========================================================================================================================================= FAILURES =========================================================================================================================================
8: ________________________________________________________________________________________________________________________ TestCreateWhileSpinning.test_client_server ________________________________________________________________________________________________________________________
8: 
8: self = <test.test_create_while_spinning.TestCreateWhileSpinning testMethod=test_client_server>
8: 
8:     def test_client_server(self):
8:         evt = threading.Event()
8:     
8:         def trigger_event(req, resp):
8:             nonlocal evt
8:             evt.set()
8:             return resp
8:     
8:         srv = self.node.create_service(BasicTypesSrv, 'foo', trigger_event)
8:         cli = self.node.create_client(BasicTypesSrv, 'foo')
8:         cli.wait_for_service()
8:         cli.call_async(BasicTypesSrv.Request())
8: >       assert evt.wait(TIMEOUT)
8: E       AssertionError: assert False
8: E        +  where False = <bound method Event.wait of <threading.Event object at 0x7fa1b4139438>>(0.5)
8: E        +    where <bound method Event.wait of <threading.Event object at 0x7fa1b4139438>> = <threading.Event object at 0x7fa1b4139438>.wait
8: 
8: test/test_create_while_spinning.py:74: AssertionError
8: _______________________________________________________________________________________________________________________ TestCreateWhileSpinning.test_guard_condition _______________________________________________________________________________________________________________________
8: 
8: self = <test.test_create_while_spinning.TestCreateWhileSpinning testMethod=test_guard_condition>
8: 
8:     def test_guard_condition(self):
8:         evt = threading.Event()
8:     
8:         guard = self.node.create_guard_condition(lambda: evt.set())
8:         guard.trigger()
8: >       assert evt.wait(TIMEOUT)
8: E       AssertionError: assert False
8: E        +  where False = <bound method Event.wait of <threading.Event object at 0x7fa1b41649e8>>(0.5)
8: E        +    where <bound method Event.wait of <threading.Event object at 0x7fa1b41649e8>> = <threading.Event object at 0x7fa1b41649e8>.wait
8: 
8: test/test_create_while_spinning.py:81: AssertionError
8: ______________________________________________________________________________________________________________________ TestCreateWhileSpinning.test_publish_subscribe ______________________________________________________________________________________________________________________
8: 
8: self = <test.test_create_while_spinning.TestCreateWhileSpinning testMethod=test_publish_subscribe>
8: 
8:     def test_publish_subscribe(self):
8:         evt = threading.Event()
8:         sub = self.node.create_subscription(BasicTypes, 'foo', lambda msg: evt.set())
8:         pub = self.node.create_publisher(BasicTypes, 'foo')
8:         pub.publish(BasicTypes())
8: >       assert evt.wait(TIMEOUT)
8: E       AssertionError: assert False
8: E        +  where False = <bound method Event.wait of <threading.Event object at 0x7fa1b4e85208>>(0.5)
8: E        +    where <bound method Event.wait of <threading.Event object at 0x7fa1b4e85208>> = <threading.Event object at 0x7fa1b4e85208>.wait
8: 
8: test/test_create_while_spinning.py:60: AssertionError
8: ____________________________________________________________________________________________________________________________ TestCreateWhileSpinning.test_timer ____________________________________________________________________________________________________________________________
8: 
8: self = <test.test_create_while_spinning.TestCreateWhileSpinning testMethod=test_timer>
8: 
8:     def test_timer(self):
8:         evt = threading.Event()
8:     
8:         tmr = self.node.create_timer(TIMEOUT / 10, lambda: evt.set())
8: >       assert evt.wait(TIMEOUT)
8: E       AssertionError: assert False
8: E        +  where False = <bound method Event.wait of <threading.Event object at 0x7fa1b41ad358>>(0.5)
8: E        +    where <bound method Event.wait of <threading.Event object at 0x7fa1b41ad358>> = <threading.Event object at 0x7fa1b41ad358>.wait
8: 
8: test/test_create_while_spinning.py:87: AssertionError
8: __________________________________________________________________________________________________________________________ TestCreateWhileSpinning.test_waitable ___________________________________________________________________________________________________________________________
8: 
8: self = <test.test_create_while_spinning.TestCreateWhileSpinning testMethod=test_waitable>
8: 
8:     def test_waitable(self):
8:         evt = threading.Event()
8:     
8:         class DummyWaitable(Waitable):
8:     
8:             def __init__(self):
8:                 super().__init__(ReentrantCallbackGroup())
8:     
8:             def is_ready(self, wait_set):
8:                 return False
8:     
8:             def take_data(self):
8:                 return None
8:     
8:             async def execute(self, taken_data):
8:                 pass
8:     
8:             def get_num_entities(self):
8:                 return NumberOfEntities(0, 0, 0, 0, 0)
8:     
8:             def add_to_wait_set(self, wait_set):
8:                 nonlocal evt
8:                 evt.set()
8:                 pass
8:     
8:         self.node.add_waitable(DummyWaitable())
8: >       assert evt.wait(TIMEOUT)
8: E       AssertionError: assert False
8: E        +  where False = <bound method Event.wait of <threading.Event object at 0x7fa1b4109908>>(0.5)
8: E        +    where <bound method Event.wait of <threading.Event object at 0x7fa1b4109908>> = <threading.Event object at 0x7fa1b4109908>.wait
8: 
8: test/test_create_while_spinning.py:115: AssertionError

@sloretz sloretz added the bug Something isn't working label May 2, 2019
@sloretz sloretz self-assigned this May 2, 2019
@sloretz
Copy link
Contributor Author

sloretz commented May 2, 2019

CI (testing rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, with one question, looks like flake8 is failing.

@@ -210,6 +210,11 @@ def executor(self, new_executor: Executor) -> None:
new_executor.add_node(self)
self.__executor_weakref = weakref.ref(new_executor)

def _wake_executor(self):
executor = self.executor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary, is there a reason for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vs

if self.executor:
   self.executor.wake()

self.executor is a property that is set to None when the node is removed from an executor. If the node is removed after the check then the call to wake might raise AttributeError

@sloretz
Copy link
Contributor Author

sloretz commented May 2, 2019

CI (testing just rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz/wake_executor_on_create_or_destroy branch from 67718c2 to 020cf7c Compare May 2, 2019 19:54
@sloretz
Copy link
Contributor Author

sloretz commented May 2, 2019

CI (testing just rclpy after rebasing on master)

Edit: WIll re-run windows CI post API freeze

@sloretz
Copy link
Contributor Author

sloretz commented May 6, 2019

CI (testing just rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz merged commit c434542 into master May 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the sloretz/wake_executor_on_create_or_destroy branch May 6, 2019 21:05
@sloretz sloretz removed the in review Waiting for review (Kanban column) label May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node.create_*() should wake executor
3 participants