-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add async run method to LaunchService #210
Conversation
ce300f6
to
e6e53b9
Compare
Rebased to solve merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes lgtm.
In general, I think this is a good direction to go in, even if we don't end up using it in the testing stuff.
I am however concerned about regressions, because this code is so tricky (the current implementation included), so I'd be in favor of merging this, but with the idea that follow up work is likely.
Do you want to move forward with getting this merged, or do you only want to merge it if we're going to be using it with the testing framework.
I share your concerns.
There's not point in having this PR collecting dust IMHO. If we regard this as a good addition, I think we should move forward and endure the pain, regardless of what happens with |
50b22bf
to
fe6c549
Compare
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
fe6c549
to
d18ddea
Compare
Hmm, Windows has failing tests, but the same show up in an unrelated CI run (ros2/rcl#477). I'll check if the issue happens on In the meantime, @wjwwood @dirk-thomas PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minimal comments, otherwise it looks great!
Signed-off-by: Michel Hidalgo <[email protected]>
Alright, going in. |
I'll keep an eye on this one, just in case nightlies go red (for some unexpected reason). |
* Add run_async coroutine to LaunchService. Signed-off-by: Michel Hidalgo <[email protected]> * Address peer review comments. Signed-off-by: Michel Hidalgo <[email protected]> * Remove duplicate thread check. Signed-off-by: Michel Hidalgo <[email protected]>
This pull request enables asynchronous launch runs. This popped up in a discussion I had with @wjwwood as an alternative to pushing a launch run to a background thread (see #126), which is unsupported (explicitly after these changes).
Consider the following snippet for an example of the new async API:
Hopefully, this will simplify launch based fixtures for testing.
cc @pbaughman for awareness.