Complete #506 backport and deprecate signal_management APIs #511

hidmic · 2021-06-16T14:54:03Z

Precisely what the title says. Connected to #506. Needs osrf/osrf_pycommon#75.

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 <michel@ekumenlabs.com>

… a socket.socket() works (#494) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-06-16T14:54:51Z

@jacobperron this is what the change to Foxy would look like. Unit tests pass locally, though I have not run full Foxy CI just yet.

felixdivo · 2021-06-17T12:25:21Z

@hidmic I could not find any docs on how to install this locally while overriding the stuff installed via apt. Do I simply clone and pip install . it globally (for the time being)?

hidmic · 2021-06-17T13:54:49Z

@hidmic I could not find any docs on how to install this locally while overriding the stuff installed via apt. Do I simply clone and pip install . it globally (for the time being)?

The simplest approach is probably to build an overlay workspace with osrf_pycommon and launch in it, checked out at the right branches.

felixdivo · 2021-06-17T15:11:35Z

Okay, I managed to install it in a more hacky way but it worked. The PR seems the fix the issue, everything seems to work fine when I issue a SIGINT via process.send_signal(SIGINT):

[WARNING] [launch]: user interrupted with ctrl-c (SIGINT)
[DEBUG] [launch.launch_context]: emitting event synchronously: 'launch.events.Shutdown'
[DEBUG] [launch]: processing event: '<launch.events.shutdown.Shutdown object at 0x7fe5c507a910>'
[DEBUG] [launch]: processing event: '<launch.events.shutdown.Shutdown object at 0x7fe5c507a910>' ✓ '<launch.event_handlers.on_shutdown.OnShutdown object at 0x7fe5c507a220>'
[DEBUG] [launch.launch_context]: emitting event synchronously: 'launch.events.process.SignalProcess'
[DEBUG] [launch]: processing event: '<launch.events.shutdown.Shutdown object at 0x7fe5c507a910>' ✓ '<launch.event_handlers.on_shutdown.OnShutdown object at 0x7fe5c519e190>'
[DEBUG] [launch]: processing event: '<launch.events.process.signal_process.SignalProcess object at 0x7fe5c507ae80>'
[DEBUG] [launch]: processing event: '<launch.events.process.signal_process.SignalProcess object at 0x7fe5c507ae80>' ✓ '<launch.event_handler.EventHandler object at 0x7fe5c4684fa0>'
[INFO] [mynode-1]: sending signal 'SIGINT' to process[mynode-1]
[INFO] [mynode-1]: process has finished cleanly [pid 16868]

hidmic · 2021-06-18T19:15:17Z

Full Foxy CI:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-06-22T12:37:29Z

Full Foxy CI after 4b7e29b:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

felixdivo · 2021-06-22T18:19:10Z

Since this is soon coming to an end thanks to @hidmic, shouldn't this be added to Foxy Patch Release 5?

jacobperron

Looks okay to me; one concern about the deprecation warnings.

jacobperron · 2021-06-22T21:33:15Z

launch/launch/utilities/signal_management.py

+
+      Use AsyncSafeSignalManager instead
+    """
+    warnings.warn(


I'd rather not release deprecation warnings into a long-standing ROS distro to avoid disruption. Besides, users who have already switched to Galactic would have already run into the API change without a tick-tock cycle. There may be other opinions.

I can only give one user's opinion (mine/my team's): I'd like to get warned of breakages as soon a possible. We'd have to change it eventually in any way since we move over to newer versions of ROS2 eventually (LTS -> LTS). And if not, a warnings.simplefilter() ist easy to implement if one does not want to see the message for the time being.

But you know best what's common practice in ROS. This isn't Rolling Ridley, after all.

I have to agree with @jacobperron ; I don't think we should introduce new deprecation warnings into a stable distribution. Having new warnings show up for Foxy users as a result of sudo apt-get update is really user-unfriendly.

When people port to Galactic or Rolling, they'll get the warnings.

Alright, see d8e320a.

However I will say that, while I agree that introducing warnings into a patch release of a stable distribution is not ideal, this patch is not transparent. Signals now go through wakeup file descriptors and handling is deferred to the main-thread local asyncio loop (assuming it is run). Depending on how the old signal management API was used (the only case in which you'd get a warning) this can potentially break those use cases -- silently as of d8e320a.

Which is why I didn't even think of backporting anything, and I probably wouldn't if these bits were not kinda broken in Foxy.

When people port to Galactic or Rolling, they'll get the warnings.

The old API was completely removed in Galactic, so no warnings (there's no way to re-implement it in terms of the new API so as to be functionally equivalent, as exposed above). This patch is a retrofit of the original patch.

I didn't fully comprehend the nature of the change. If there's a risk that we're breaking somebody, then I think issuing a warning is a good thing. I don't like the idea of releasing breaking changes in Foxy, but if you think the fix is worth the potential churn then I'd rather be very loud about it.

So, without this fix, launch hangs every now and then. It's the same we've experienced with ros2cli tests before. And it hit @felixdivo, so it may hit others too.

I agree about being loud. I thought a warning would be OK, but perhaps it's too loud. @clalancette ?

It seems like we are stuck between a rock and a hard place. While I don't like introducing new warnings into a stable distribution, silently breaking users isn't good either.

In this case, I'll defer to your best judgement on what to do here. You obviously have a much better handle on the issue and launch in general than I do.

Presumably, the these signal APIs are not very popular so I'd hope the number of affected users would be small.

Alright, I put deprecation warnings back in. I'll merge this PR into #506 and run CI there.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

This reverts commit d8e320a. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic and others added 6 commits June 16, 2021 10:21

Workaround asyncio signal handling on Unix (#479)

9bc5fea

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 <michel@ekumenlabs.com>

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

20c21a6

… a socket.socket() works (#494) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

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

31b3277

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

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

90c1cba

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Bring back deprecated launch.utilities.signal_management APIs

8ca87d8

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Have launch.utilities.signal_management tests passing

56942c2

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested review from jacobperron and mjeronimo June 16, 2021 14:54

hidmic mentioned this pull request Jun 16, 2021

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

Merged

felixdivo mentioned this pull request Jun 18, 2021

Failure to quit ros2 launch via SIGINT #495

Closed

vatanaksoytezer mentioned this pull request Jun 19, 2021

Fix most servo tests, cleanup moveit/moveit2#511

Closed

No SIGQUIT on Windows

4b7e29b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Skip SIGQUIT test on Windows

9b88259

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

jacobperron reviewed Jun 22, 2021

View reviewed changes

hidmic added 2 commits June 23, 2021 12:32

Drop deprecation warnings

d8e320a

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Revert "Drop deprecation warnings"

7a89f67

This reverts commit d8e320a. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic merged commit f21e21c into mjeronimo/backport-476-to-foxy Jun 29, 2021

delete-merged-branch bot deleted the hidmic/adapt/mjeronimo/backport-476-to-foxy branch June 29, 2021 15:35

Complete #506 backport and deprecate signal_management APIs #511

Complete #506 backport and deprecate signal_management APIs #511

Uh oh!

Conversation

hidmic commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jun 16, 2021

Uh oh!

felixdivo commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jun 17, 2021

Uh oh!

felixdivo commented Jun 17, 2021

Uh oh!

hidmic commented Jun 18, 2021

Uh oh!

hidmic commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixdivo commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hidmic commented Jun 16, 2021 •

edited

Loading

felixdivo commented Jun 17, 2021 •

edited

Loading

hidmic commented Jun 22, 2021 •

edited

Loading

felixdivo commented Jun 22, 2021 •

edited

Loading

hidmic Jun 23, 2021 •

edited

Loading