[Foxy] Handle signals within the asyncio loop. (#476) #506

mjeronimo · 2021-05-04T18:45:12Z

Signed-off-by: Michel Hidalgo [email protected]

Backporting #476 to Foxy.
Fixes: ros2/launch_ros#223

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2021-05-04T18:53:47Z

If we backport #476, we have to backport #479, #494, #497, and #498 too. Signal handling is tricky (so much so that CPython itself is quite rough there).

mjeronimo · 2021-05-04T18:56:30Z

@hidmic Do you recommend proceeding with those for Foxy? Is it worth it for this issue?

hidmic · 2021-05-04T19:06:31Z

@mjeronimo all those PRs patch #476 in one way or another. #479 deals with an issue in asyncio that became a problem when signal management implementation changed in #476. #494, #497, and #498 fix issues in #476 in various ways (mainly for Windows).

This is why I did not proactively backport these changes. But ros2/launch_ros#223 suggests it may be worth the risk. #495 may be running into similar issues.

mjeronimo · 2021-05-04T20:26:07Z

@jacobperron What do you think about backporting this set of changes to Foxy? Would like to get your input before proceeding.

jacobperron · 2021-05-04T20:40:44Z

I'm okay with backporting all of the changes pointed out, as long as @hidmic doesn't have any major reservations about them. On the surface of it, it looks like it's worth the effort to improve things in Foxy, but I don't know enough about the signal handling changes to say if we're going to break users code.

Of all of the changes, the only thing I would change for Foxy would be to keep this function (unused) to maintain API compatibility:

https://github.com/ros2/launch/pull/494/files#diff-5dcef8ac9a56749cd4f33305fea85b4c918383b74c8c1369cce3a22286d84ee9L30

hidmic · 2021-05-04T20:46:01Z

as long as @hidmic doesn't have any major reservations about them

No particular reservations. It's sensitive code, that's all (4 PRs followed #476).

Of all of the changes, the only thing I would change for Foxy would be to keep this function (unused) to maintain API compatibility:

That function didn't exist before #476, so it doesn't exist in Foxy. On the other hand, this patch ~~removes quite a few functions in~~ changes signal_management.py completely. Off the top of my head, I don't think we can keep that API around. Maybe as a no-op?

felixdivo · 2021-05-07T08:44:46Z

I can probably offer to help with testing these changes. It can't be too hard to set it up properly.

felixdivo · 2021-05-15T11:36:01Z

The fix would be important for me / my team. Can I assist you in some way?

Keeping the old API and having it do nothing seems like the only sensible way (the only one with moderate coding effort). One should perhaps also print a warning pointing to this PR or otherwise explaining that the functionality has moved and the function won't do anything any more. That way, there is actually merit in keeping it.

jacobperron

I overlooked the changes in the PR originally (and only looked at the follow-up fixes).

I'm very hesitant about backporting this to Foxy since it breaks API. I think we should investigate either:

Making an API-compatible fix here (or in launch_ros).
Keeping the old API, and keep it functional.

I don't like the idea of making the old API a noop (even if it produces a warning). This can cause a subtle break for the user versus a hard break (if the API does not exist). If we are going to produce a warning, we may as well remove the old API since it forces the user to update instead of potentially having this break subtly.

jacobperron · 2021-05-17T19:16:02Z

launch/launch/utilities/__init__.py

-    'install_signal_handlers',
-    'on_sigint',
-    'on_sigquit',
-    'on_sigterm',


Removing these APIs will break anyone using them directly.

hidmic · 2021-05-19T16:14:14Z

Keeping the old API, and keep it functional.

Hmm, perhaps we can use the fact that asyncio loops are thread-local globals and re-implement this API with a global AsyncSignalSafeManager instance. I haven't thought through all the implications though.

felixdivo · 2021-06-16T10:23:04Z

I just want to ping this issue and find out what the roadmap might be. Is there someone willing to tackle this? I of course unterstand, that this is a rather nasty change and personally also do not want to dig into it.

So if the decision is taken that there simply will not be a fix and foxy will be left with the bug ☹️, it would be great to spell it out in this issue. That way, any stakeholder knows what to do. I my case, I will probably have to hack something together in our product until the next ROS2 LTS is released.

hidmic · 2021-06-16T14:56:57Z

I just put together a POC in #511. It needs testing, and Foxy ROS Boss approval.

Unix asyncio loops override existing signal wakeup file descriptors with no regards for existing ones -- set by user code or by another loop Signed-off-by: Michel Hidalgo <[email protected]>

… a socket.socket() works (#494) Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

This reverts commit d8e320a. Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2021-06-29T15:39:06Z

Full Foxy CI:

Linux
Linux-aarch64
macOS
Windows

felixdivo · 2021-06-29T17:17:13Z

As with #511, this PR should probably be added to the Foxy Patch Release 5

hidmic · 2021-06-29T22:05:26Z

Test failures seem unrelated. Are we good to go @jacobperron @mjeronimo ?

mjeronimo · 2021-06-29T23:47:27Z

Looks good to me. Thanks for taking on this complicated issue.

hidmic · 2021-06-30T22:58:42Z

@jacobperron I'd like your blessing before I merge this.

felixdivo · 2021-07-05T18:21:44Z

Where should I look for a possible release date of the Foxy Patch Release 5 that this will be part of?

hidmic · 2021-07-07T20:34:01Z

Alright, moving forward !

Handle signals within the asyncio loop. (#476)

f0e4642

Signed-off-by: Michel Hidalgo <[email protected]>

mjeronimo requested review from hidmic and jacobperron May 4, 2021 18:45

jacobperron reviewed May 17, 2021

View reviewed changes

clalancette assigned mjeronimo Jun 3, 2021

hidmic mentioned this pull request Jun 16, 2021

Complete #506 backport and deprecate signal_management APIs #511

Merged

vatanaksoytezer mentioned this pull request Jun 19, 2021

Fix most servo tests, cleanup moveit/moveit2#511

Closed

hidmic and others added 10 commits June 29, 2021 12:35

Workaround asyncio signal handling on Unix (#479)

b24d02c

Unix asyncio loops override existing signal wakeup file descriptors with no regards for existing ones -- set by user code or by another loop Signed-off-by: Michel Hidalgo <[email protected]>

Remove is_winsock_handle() and instead test if wrapping the handle in…

db25413

… a socket.socket() works (#494) Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Close the socket pair used for signal management (#497)

4fd532a

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Only try to wrap the fd in a socket on Windows (#498)

365402a

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Bring back deprecated launch.utilities.signal_management APIs

947b00e

Signed-off-by: Michel Hidalgo <[email protected]>

Have launch.utilities.signal_management tests passing

2b65b9d

Signed-off-by: Michel Hidalgo <[email protected]>

No SIGQUIT on Windows

5918db6

Signed-off-by: Michel Hidalgo <[email protected]>

Skip SIGQUIT test on Windows

dc175ca

Signed-off-by: Michel Hidalgo <[email protected]>

Drop deprecation warnings

0a817c1

Signed-off-by: Michel Hidalgo <[email protected]>

Revert "Drop deprecation warnings"

f21e21c

This reverts commit d8e320a. Signed-off-by: Michel Hidalgo <[email protected]>

hidmic requested a review from jacobperron July 7, 2021 14:13

hidmic approved these changes Jul 7, 2021

View reviewed changes

jacobperron approved these changes Jul 7, 2021

View reviewed changes

hidmic merged commit 4c60ea0 into foxy Jul 7, 2021

delete-merged-branch bot deleted the mjeronimo/backport-476-to-foxy branch July 7, 2021 20:35

rbonghi mentioned this pull request Aug 24, 2021

Catch SIGTERM from nanosaur nodes rnanosaur/nanosaur#27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Foxy] Handle signals within the asyncio loop. (#476) #506

[Foxy] Handle signals within the asyncio loop. (#476) #506

mjeronimo commented May 4, 2021

hidmic commented May 4, 2021 •

edited

Loading

mjeronimo commented May 4, 2021

hidmic commented May 4, 2021 •

edited

Loading

mjeronimo commented May 4, 2021

jacobperron commented May 4, 2021

hidmic commented May 4, 2021 •

edited

Loading

felixdivo commented May 7, 2021

felixdivo commented May 15, 2021 •

edited

Loading

jacobperron left a comment

jacobperron May 17, 2021

hidmic commented May 19, 2021

felixdivo commented Jun 16, 2021

hidmic commented Jun 16, 2021

hidmic commented Jun 29, 2021

felixdivo commented Jun 29, 2021

hidmic commented Jun 29, 2021

mjeronimo commented Jun 29, 2021

hidmic commented Jun 30, 2021

felixdivo commented Jul 5, 2021

hidmic commented Jul 7, 2021

[Foxy] Handle signals within the asyncio loop. (#476) #506

[Foxy] Handle signals within the asyncio loop. (#476) #506

Conversation

mjeronimo commented May 4, 2021

hidmic commented May 4, 2021 • edited Loading

mjeronimo commented May 4, 2021

hidmic commented May 4, 2021 • edited Loading

mjeronimo commented May 4, 2021

jacobperron commented May 4, 2021

hidmic commented May 4, 2021 • edited Loading

felixdivo commented May 7, 2021

felixdivo commented May 15, 2021 • edited Loading

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron May 17, 2021

Choose a reason for hiding this comment

hidmic commented May 19, 2021

felixdivo commented Jun 16, 2021

hidmic commented Jun 16, 2021

hidmic commented Jun 29, 2021

felixdivo commented Jun 29, 2021

hidmic commented Jun 29, 2021

mjeronimo commented Jun 29, 2021

hidmic commented Jun 30, 2021

felixdivo commented Jul 5, 2021

hidmic commented Jul 7, 2021

hidmic commented May 4, 2021 •

edited

Loading

hidmic commented May 4, 2021 •

edited

Loading

hidmic commented May 4, 2021 •

edited

Loading

felixdivo commented May 15, 2021 •

edited

Loading