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

Handle signals within the asyncio loop. #476

Merged
merged 9 commits into from
Jan 12, 2021
Merged

Handle signals within the asyncio loop. #476

merged 9 commits into from
Jan 12, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Dec 16, 2020 •
edited
Loading

Connected to #473. This patch uses signal.set_wakeup_fd to delegate signal handling to the running loop. Similar to asyncio._UnixSelectorEventLoop.add_signal_handler() implementation, but not limited to Unix platforms.

For additional context, without this patch, in a scenario like the one in #473, events emitted within a signal handler do not seem to wake up the loop.

@hidmic
Handle signals within the asyncio loop.
e88ad29 
Similar to asyncio._UnixSelectorEventLoop.add_signal_handler()
implementation, but not limited to Unix platforms.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic changed the title Handle signals within the asyncio loop. [POC] Handle signals within the asyncio loop. Dec 16, 2020
@hidmic
Copy link
Contributor Author

hidmic commented Dec 16, 2020

This is lacking tests and quite a bit of documentation. But I wanted to share this POC as soon as possible for thorough scrutiny.

wjwwood
wjwwood approved these changes Dec 17, 2020
View reviewed changes
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.

The changes make sense to me, but I haven't dug deeply into the signal handler provided by the loop and I haven't tried it out locally.

This is good for me, if we can show it works well on all the platforms.

Have you tried it when the launch service is not in the main thread?

@wjwwood
Copy link
Member

wjwwood commented Dec 17, 2020

And yes, it needs docs and tests if we can manage to make them (can be hard with these kind of things).

@hidmic
Add AsyncSafeSignalManager test
4071f5d 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add AsyncSafeSignalManager documentation
7823301 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Update LaunchService documentation
bc92085 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic changed the title [POC] Handle signals within the asyncio loop. Handle signals within the asyncio loop. Dec 23, 2020
@hidmic
Copy link
Contributor Author

hidmic commented Dec 23, 2020 •
edited
Loading

Have you tried it when the launch service is not in the main thread?

It won't work, as signal handling API is restricted to the main thread. But that's a limitation we already had. See here and here). Also #126.

FWIW I think we should eventually drop signal handling within launch.LaunchService. Some async-signal-safe APIs for a user to setup their own handlers would pose less restrictions on usage. Moreover, if we could replace signals with a less rudimentary IPC to monitor and interrupt subprocesses, that'd lift all concurrency limitations and improve launch behavior on Windows.

@hidmic
Copy link
Contributor Author

hidmic commented Dec 23, 2020

CI up to launch, test_launch_ros, and test_communication:

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

@hidmic
Copy link
Contributor Author

hidmic commented Dec 23, 2020

If this patch gets merged, I'd like to see if it fixes ros2/build_farmer#248.

ivanpauno
ivanpauno approved these changes Dec 23, 2020
View reviewed changes
launch/launch/utilities/signal_management.py Outdated Show resolved Hide resolved
@hidmic
Address peer review comments
3c753e6 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Deal with Windows quirks.
3f9ce9c 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ivanpauno
ivanpauno approved these changes Dec 23, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Code too handle the windows quirks seems reasonable too me!

@hidmic
Deal with more Windows quirks.
71ab3f2 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Dec 23, 2020

Code too handle the windows quirks seems reasonable too me!

Of course, it wasn't enough... See 71ab3f2.

@hidmic
Copy link
Contributor Author

hidmic commented Dec 23, 2020 •
edited
Loading

CI up to launch, test_launch_ros, and test_communication:

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

@hidmic
No SIGQUIT on Windows.
652e7b5 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Use msvcrt.get_osfhandle() to detect WinSock handles.
584df2f 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic mentioned this pull request Jan 12, 2021
Merged
@hidmic
Copy link
Contributor Author

hidmic commented Jan 12, 2021

CI up to launch, test_launch_ros, test_communication, and osrf_pycommon:

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

@hidmic
Copy link
Contributor Author

hidmic commented Jan 12, 2021

Going in! As a side note, we should sort out asyncio usage in launch at some point. It goes great lengths to ensure loop locality, and asyncio API is set up for global loops.

@hidmic hidmic merged commit 3d77b5d into master Jan 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/fix-signal-handling branch January 12, 2021 21:54
@hidmic hidmic mentioned this pull request Jan 12, 2021
Open
@hidmic hidmic mentioned this pull request Apr 12, 2021
Closed
@jacobperron jacobperron mentioned this pull request Apr 27, 2021
Closed
@hidmic hidmic mentioned this pull request May 4, 2021
Closed
mjeronimo pushed a commit that referenced this pull request May 4, 2021
@hidmic @mjeronimo
Handle signals within the asyncio loop. (#476)
f0e4642 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@mjeronimo mjeronimo mentioned this pull request May 4, 2021
Merged
@vatanaksoytezer vatanaksoytezer mentioned this pull request Jun 19, 2021
Closed
hidmic pushed a commit that referenced this pull request Jul 7, 2021
@mjeronimo
[Foxy] Handle signals within the asyncio loop. (#506)
4c60ea0 
* Handle signals within the asyncio loop. (#476)
* Workaround asyncio signal handling on Unix (#479)
* Remove is_winsock_handle() and instead test if wrapping the handle in a socket.socket() works (#494)
* Close the socket pair used for signal management (#497)
* Only try to wrap the fd in a socket on Windows (#498)
* Bring back deprecated launch.utilities.signal_management APIs

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ciandonovan
Copy link

@hidmic This merge broke SIGTERM handling in Launch by removing the signal handler and not replacing it.

SIGINT wasn't broken only because Python installs a signal handler for it by default, translating it into a KeyboardInterrupt exception.

Issue with further details: #666
Pull request fixing it: #712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@wjwwood wjwwood wjwwood approved these changes

@jacobperron jacobperron Awaiting requested review from jacobperron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hidmic @wjwwood @ciandonovan @ivanpauno