signal: add SignalKind::info on illumos #6995

sunshowers · 2024-11-28T03:23:06Z

Motivation

illumos has supported SIGINFO for a long time; see illumos/illumos-gate@19d32b9. It is possible to create a signal handler from the constant by hand, but having SignalKind::info work is more convenient.

Solution

Add target_os = "illumos" to the cfg for SignalKind::info.

Darksonn · 2024-11-28T10:16:07Z

Looks reasonable to me.

cc @hawkw for illumos expertise

sunshowers · 2024-12-03T08:49:17Z

Hmm, so trying to use this further I ran into a "signal too large" issue.

SIGINFO is signal 41 on illumos, but this code only supports signal numbers up to 33. Is that deliberate? Seems like at least on illumos it should go up to 41.

edit: https://github.com/illumos/illumos-gate/blob/27ecbff00d8c86a2647d6fe325cacb220d712115/usr/src/uts/common/sys/iso/signal_iso.h#L51-L94 -- these are all the signals on illumos. Some of the higher signals are unlikely to be used but SIGINFO is ctrl-T just like it is on BSDs, so is quite useful. I think I'll submit a change to also bump this up to 41 on illumos.

sunshowers · 2024-12-03T08:56:37Z

~~Hmm, other platforms also have real-time signals available -- is there a reason that's restricted to Linux?~~

Ah, it looks like it's just Linux, illumos and GNU Hurd at the moment.

Darksonn · 2024-12-03T11:03:52Z

@ipetkov Thoughts on the signal range?

Darksonn · 2024-12-03T11:04:37Z

Does the "signal too large" issue happen when you run the Tokio test suite on illumos?

ipetkov · 2024-12-03T16:48:16Z

Hi @sunshowers thanks for the PR!

We've had a heuristic in place for ages that checks the signal range to avoid any debugging headaches across platforms (in case a signal doesn't exist and it appears to never fire):

tokio/tokio/src/signal/unix.rs

Lines 25 to 33 in dcae2b9

    
           // There are reliable signals ranging from 1 to 33 available on every Unix platform. 
        
           #[cfg(not(target_os = "linux"))] 
        
           let possible = 0..=33; 
        
           // On Linux, there are additional real-time signals available. 
        
           #[cfg(target_os = "linux")] 
        
           let possible = 0..=libc::SIGRTMAX(); 
        
           possible.map(|_| SignalInfo::default()).collect()

Obviously Illumos support was missed there but something we should fix while we're here!

EDIT: heh you already found it, should have read the whole thread before responding. Happy for that check to be updated to whatever value is most appropriate for Illumos

illumos has supported SIGINFO for a long time; see illumos/illumos-gate@19d32b9.

sunshowers · 2024-12-03T20:57:07Z

Updated the PR with:

the signal count bumped up to 41 on illumos
a note about real-time signals on illumos
a test for SIGINFO since I didn't see any. I guess we'd ideally have separate tests for each possible signal on each platform, but this one will do for now I hope.

sunshowers · 2024-12-03T21:00:54Z

Does the "signal too large" issue happen when you run the Tokio test suite on illumos?

It did not with the initial version of the PR -- have added a test for this.

hawkw

Overall, this seems good to me! I had a couple very minor suggestions.

hawkw · 2024-12-03T21:15:29Z

tokio/src/signal/unix.rs

+        // On illumos, signal numbers go up to 41 (SIGINFO). The list of signals
+        // hasn't changed since 2013. See
+        // https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/iso/signal_iso.h.
+        //
+        // illumos also has real-time signals, but (a) the number of real-time
+        // signals is actually configurable at runtime and (b) this capability
+        // isn't exposed by libc as of 0.2.167, so we don't support them at the
+        // moment. If support for real-time signals on illumos is desired, this
+        // code would have to be changed to accommodate that.


Thanks for including the comment, this is lovely!

Out of interest, there wouldn't happen to be any libc ticket or similar tracking support for real-time issues? Not a big deal, but if there is something, it could be worth referencing that?

No, I couldn't find a libc issue.

Put up rust-lang/libc#4171 -- will update this comment with a link to that.

Added a link -- once it lands in upstream libc, it should be possible to do on illumos what's done on Linux.

hawkw · 2024-12-03T21:17:05Z

tokio/tests/signal_info.rs

+
+    let _ = fire.send(());
+
+    sig.recv().await.expect("received SIGINFO signal");


Should we consider setting a (very long) timeout on this, so that the test failure mode if the signal is not received is a panic, rather than running forever? Just a thought.

I copied the other signal tests which don't set that kind of timeout, but it would certainly make sense to do so. Alternatively, the timeout could be set in nextest. What do you think?

We expect tests to run with cargo test too, so we shouldn't use nextest specific features. Having a timeout sgtm.

All right, done.

Darksonn · 2024-12-04T09:26:27Z

tokio/tests/signal_info.rs

+    // NB: simulate a signal coming in by exercising our signal handler
+    // to avoid complications with sending SIGINFO to the test process
+    tokio::spawn(async {
+        wait.await.expect("wait failed");
+        send_signal(libc::SIGINFO);
+    });


The oneshot channel changes nothing. Tests use the single-threaded runtime, so this spawned task is going to run on this thread, and it will not run until the main test task yields, which it does after polling sig.recv() once.

Was that the intent of this code?

I copied this from the other tests, specifically:

tokio/tokio/tests/signal_ctrl_c.rs

Lines 19 to 26 in c032ea0

let (fire, wait) = oneshot::channel();

// NB: simulate a signal coming in by exercising our signal handler

// to avoid complications with sending SIGINT to the test process

tokio::spawn(async {

wait.await.expect("wait failed");

send_signal(libc::SIGINT);

});

I agree that it doesn't make a difference in the narrow case of a single-threaded runtime.

Would you like to keep the oneshot channel, have me remove it from this test, or do it yourself across all the signal tests?

Filed #7015.

…1938) Part 1 of work to perform interactive queries. With this change, if nextest receives either SIGINFO (where available) or SIGUSR1, it will print a list of all currently running tests with their states. The full test state machine is modeled in the event responses, along with possible sources of errors. I tried making Ctrl-Break on Windows do the same thing, but unfortunately that doesn't quite work in practice. That's because Ctrl-Break is received by all processes connected to the console, not just by the equivalent of the "foreground process group". The printing out of info works, but running tests are immediately aborted as well. So remove this. (In interactive terminals we'll support an alternative -- pressing `i` -- which should work for most use cases on Windows.) There is also an undocumented environment variable `__NEXTEST_SIGQUIT_AS_INFO` which allows `SIGQUIT` (`Ctrl-\`) to perform an info query. Quite useful for testing on Linux, where SIGINFO is sadly unavailable. Note: illumos does support SIGINFO, but that is blocked on an upstream issue: tokio-rs/tokio#6995

Darksonn added A-tokio Area: The main tokio crate M-signal Module: tokio/signal labels Nov 28, 2024

sunshowers force-pushed the info-illumos branch from 58ad99d to c69052a Compare December 3, 2024 20:53

signal: add SignalKind::info on illumos

e548233

illumos has supported SIGINFO for a long time; see illumos/illumos-gate@19d32b9.

sunshowers force-pushed the info-illumos branch from c69052a to e548233 Compare December 3, 2024 20:54

hawkw approved these changes Dec 3, 2024

View reviewed changes

sunshowers added 2 commits December 3, 2024 22:06

add link to libc PR

4f0d757

add timeout

3e2cda5

ipetkov approved these changes Dec 3, 2024

View reviewed changes

Darksonn reviewed Dec 4, 2024

View reviewed changes

sunshowers mentioned this pull request Dec 4, 2024

[nextest-runner] handle SIGINFO and SIGUSR1 to dump info to screen nextest-rs/nextest#1938

Merged

Darksonn merged commit 480c010 into tokio-rs:master Dec 5, 2024
82 checks passed

sunshowers deleted the info-illumos branch December 5, 2024 18:42

sunshowers mentioned this pull request Dec 11, 2024

signal: add support for realtime signals on illumos #7029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signal: add SignalKind::info on illumos #6995

signal: add SignalKind::info on illumos #6995

sunshowers commented Nov 28, 2024

Darksonn commented Nov 28, 2024

sunshowers commented Dec 3, 2024 •

edited

Loading

sunshowers commented Dec 3, 2024 •

edited

Loading

Darksonn commented Dec 3, 2024

Darksonn commented Dec 3, 2024

ipetkov commented Dec 3, 2024 •

edited

Loading

sunshowers commented Dec 3, 2024 •

edited

Loading

sunshowers commented Dec 3, 2024

hawkw left a comment

hawkw Dec 3, 2024

sunshowers Dec 3, 2024

sunshowers Dec 3, 2024 •

edited

Loading

sunshowers Dec 3, 2024

hawkw Dec 3, 2024

sunshowers Dec 3, 2024

Darksonn Dec 3, 2024

sunshowers Dec 3, 2024

Darksonn Dec 4, 2024

sunshowers Dec 4, 2024

sunshowers Dec 4, 2024

Darksonn Dec 5, 2024


		let _ = fire.send(());

		sig.recv().await.expect("received SIGINFO signal");

	let (fire, wait) = oneshot::channel();

	// NB: simulate a signal coming in by exercising our signal handler
	// to avoid complications with sending SIGINT to the test process
	tokio::spawn(async {
	wait.await.expect("wait failed");
	send_signal(libc::SIGINT);
	});

signal: add SignalKind::info on illumos #6995

signal: add SignalKind::info on illumos #6995

Conversation

sunshowers commented Nov 28, 2024

Motivation

Solution

Darksonn commented Nov 28, 2024

sunshowers commented Dec 3, 2024 • edited Loading

sunshowers commented Dec 3, 2024 • edited Loading

Darksonn commented Dec 3, 2024

Darksonn commented Dec 3, 2024

ipetkov commented Dec 3, 2024 • edited Loading

sunshowers commented Dec 3, 2024 • edited Loading

sunshowers commented Dec 3, 2024

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Dec 3, 2024 •

edited

Loading

sunshowers commented Dec 3, 2024 •

edited

Loading

ipetkov commented Dec 3, 2024 •

edited

Loading

sunshowers commented Dec 3, 2024 •

edited

Loading

sunshowers Dec 3, 2024 •

edited

Loading