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

Fixes timer global init of event handler #184

Merged
merged 4 commits into from
Feb 14, 2019
Merged

Conversation

pbaughman
Copy link
Contributor

Fixes #183

  • Use an attribute on the context to see if we need to install the timer
    callback event handler
  • Add a regression test that fails (hangs, actually) before the fix was applied

I manually verified that a launch description containing multiple timers would only register the TimerEvent handler once.

 - Use an attribute on the context to see if we need to install the timer
   callback event handler
 - Add a regression test that fails (hangs, actually) before the fix was applied
@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 13, 2019
@pbaughman pbaughman changed the title Fixes timer global init of event handler ISSUE183 Fixes timer global init of event handler Feb 13, 2019
launch/launch/actions/timer_action.py Outdated Show resolved Hide resolved
launch/test/launch/test_timer_action.py Outdated Show resolved Hide resolved
@wjwwood
Copy link
Member

wjwwood commented Feb 13, 2019

Other than one small implementation detail, this lgtm.

@wjwwood wjwwood changed the title ISSUE183 Fixes timer global init of event handler Fixes timer global init of event handler Feb 13, 2019
Co-Authored-By: pbaughman <dblue135@yahoo.com>
@wjwwood
Copy link
Member

wjwwood commented Feb 13, 2019

CI:

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

@pbaughman pbaughman mentioned this pull request Feb 13, 2019
1 task
@pbaughman
Copy link
Contributor Author

Hah - my different version of flake8 strikes again. . .

./test/launch/test_timer_action.py:19:1: E302 expected 2 blank lines, found 1
def test_multiple_launch_with_timers():
^

./test/launch/test_timer_action.py:27:24: Q000 Remove bad quotes
                period="1",
                       ^

./test/launch/test_timer_action.py:29:52: Q000 Remove bad quotes
                    launch.actions.Shutdown(reason="Timer expired")
                                                   ^

./launch/actions/timer_action.py:138:33: Q000 Remove bad quotes
        if not hasattr(context, "_TimerAction__event_handler_has_been_installed"):
                                ^

./launch/actions/timer_action.py:148:30: Q000 Remove bad quotes
            setattr(context, "_TimerAction__event_handler_has_been_installed", True)
                             ^

1     E302 expected 2 blank lines, found 1
4     Q000 Remove bad quotes

143 files checked
5 errors

'E'-type errors: 1
'Q'-type errors: 4

@wjwwood
Copy link
Member

wjwwood commented Feb 14, 2019

That might also be that you're missing some of the flake8 extensions we use, but I'm not sure. I think the quote style one is an extension (again not 100% sure, but I think I remember that being the case).

@wjwwood
Copy link
Member

wjwwood commented Feb 14, 2019

I'll rerun CI quickly if you can fix those up, so we can unblock #181.

@wjwwood
Copy link
Member

wjwwood commented Feb 14, 2019

CI:

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

@wjwwood wjwwood merged commit 67c2006 into ros2:master Feb 14, 2019
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Feb 14, 2019
@pbaughman pbaughman deleted the issue183 branch February 14, 2019 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timer_action use of global variable prevents multiple launches from same process
3 participants