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.

Sign up for GitHub

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

Jump to bottom

Move OnProcessStart from LoadComposableNodes to ComposableNodeContainer #16

Merged
merged 3 commits into from
Apr 19, 2019
Merged

Move OnProcessStart from LoadComposableNodes to ComposableNodeContainer #16

merged 3 commits into from
Apr 19, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 17, 2019

Currently LoadComposableNodes.execute() returns an action that loads the nodes when the container process starts. However, if the container process is already started then these loads will never be loaded because the event has already been triggered. This changes LaunchComposableNodes.execute to launch the nodes right away, and makes ComposableNodeContainer return an action that triggers LaunchComposableNodes on the event OnProcessStart.

The result is LoadComposableNodes can be triggered by some event long after the container process starts.

I think this can be merged as is, though #15 has a test for this case. To run that you'll need to make local a branch with this + #8 + #15.

@sloretz
Move OnProcessStart from Load to Container
f31b8ca 
This allows LoadComposableNodes to be triggered by some event long after
the container process starts.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz requested a review from hidmic April 17, 2019 15:42
@sloretz sloretz self-assigned this Apr 17, 2019
@sloretz sloretz mentioned this pull request Apr 17, 2019
Open
@sloretz
remove unused imports
3b039cb 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 17, 2019

CI (testing launch_ros and test_launch_ros)

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

hidmic
hidmic approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM though I haven't tested it locally. Thanks for the fix @sloretz !

BTW could we document this assumption in the LoadComposableNodes action i.e. that the container is running?

@sloretz
Document LodeComposableNode must execute after container
fd4ece6 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 18, 2019

@hidmic Documented in fd4ece6

CI linux (just one platform for linter tests since change is just in a docstring)
Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Apr 19, 2019

I'll assume @hidmic's approval extends to the documentation added in fd4ece6 and merge this

@sloretz sloretz merged commit 00ef7ca into master Apr 19, 2019
@sloretz sloretz deleted the sloretz/load_composable_nodes_on_event branch April 19, 2019 20:56
@sloretz sloretz removed the in review label Apr 19, 2019
jacobperron added a commit that referenced this pull request Aug 10, 2020
@jacobperron
Do not use event handler for loading composable nodes
c08b666 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Aug 10, 2020
@jacobperron
Do not use event handler for loading composable nodes
6f9ea34 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Aug 11, 2020
@jacobperron
Do not use event handler for loading composable nodes
dcb2637 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This was referenced Aug 11, 2020
Merged
Closed
jacobperron added a commit that referenced this pull request Aug 17, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
74b3fbf 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
d03beed 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
f2fbcf8 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
e610772 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
2bd9909 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
e422c4e 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 19, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
d78badd 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 20, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170)
0c4b7ba 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Nov 30, 2020
@jacobperron
Do not use event handler for loading composable nodes (#170) (#208)
10241c1 
Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@sloretz sloretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sloretz @hidmic