Skip to content

Implement PosixSignal API - #54136

Merged
stephentoub merged 65 commits into
dotnet:mainfrom
tmds:posix_signal
Jul 8, 2021
Merged

Implement PosixSignal API#54136
stephentoub merged 65 commits into
dotnet:mainfrom
tmds:posix_signal

Conversation

@tmds

@tmdstmds commented Jun 14, 2021

Copy link
Copy Markdown
Member

Implements #50527.

I still need to add tests.

Can you do a first review of the implementation?

cc @janvorli@jkotas@stephentoub@davidfowl@bartonjs

@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I've manually added these to the ref file, I don't know if there is an automated way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The automated way is documented in https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md . It would be a good idea to rerun the automated process to make sure that the formatting is right.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But beware, it currently removes attributes...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I ran it and copied in the PosixSignal bits.
I'm surprised to see internal instead of private on the PosixSignalRegistration constructor.

Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
@andyleejordan

Copy link
Copy Markdown
Member

Hey, I just wanted to drop by and say it's really exciting to see this work! 🥳

Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs Outdated
@tmds

tmds commented Jun 18, 2021

Copy link
Copy Markdown
MemberAuthor

The implementation has changed a bit since the initial PR.

  • Positive PosixSignal values can now be used as raw signal numbers.
  • Terminal configuration on SIGCONT/SIGCHLD can be canceled, as requested by @alexrp.

The implementation will call the original sa_handler/sa_sigaction if one was set.

It handles specifically what is cancelable by the runtime using PosixSignal API:

  • Not terminating for SIGTERM/SIGINT/SIGQUIT
  • Skipping terminal configuration for SIGCHLD/SIGCONT.

I'll look at writing some tests next week.

@alexrp

Copy link
Copy Markdown
Contributor

Just posting here as well so it doesn't get missed: I think it's worth considering a rename to UnixSignal for consistency reasons; see this comment.

Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
Comment threadsrc/libraries/Native/Unix/System.Native/pal_signal.c Outdated
@lambdageek

Copy link
Copy Markdown
Member

@tmds what's the expected behavior if users set up a handler for a signal that the runtime uses internally.

For example, the mono GC uses signals on non-Apple Unixes to suspend threads that are in preemptive mode - we pick the signal dynamically and use pthread_kill for signalling.

staticintsuspend_signal_num=-1;
staticintrestart_signal_num=-1;
staticintabort_signal_num=-1;
staticsigset_tsuspend_signal_mask;
staticsigset_tsuspend_ack_signal_mask;
staticvoid
signal_add_handler (intsigno, void (*handler)(int, siginfo_t*, void*), intflags)
{
structsigactionsa;
intret;
sa.sa_sigaction=handler;
sigfillset (&sa.sa_mask);
sa.sa_flags=SA_SIGINFO | flags;
ret=sigaction (signo, &sa, NULL);
g_assert (ret!=-1);
}
staticint
abort_signal_get (void)
{
#if defined(HOST_ANDROID)
returnSIGTTIN;
#elif defined (__OpenBSD__)
returnSIGUSR1;
#elif defined (SIGRTMIN)
staticintabort_signum=-1;
if (abort_signum==-1)
abort_signum=mono_threads_suspend_search_alternative_signal ();
returnabort_signum;
#elif defined (SIGTTIN)
returnSIGTTIN;
#else
g_error ("unable to get abort signal");
#endif
}
staticint
suspend_signal_get (void)
{
#if defined(HOST_ANDROID)
returnSIGPWR;
#elif defined (SIGRTMIN)
staticintsuspend_signum=-1;
if (suspend_signum==-1)
suspend_signum=mono_threads_suspend_search_alternative_signal ();
returnsuspend_signum;
#else
#if defined(__APPLE__) || defined(__OpenBSD__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
returnSIGXFSZ;
#else
returnSIGPWR;
#endif
#endif
}
staticint
restart_signal_get (void)
{
#if defined(HOST_ANDROID)
returnSIGXCPU;
#elif defined (SIGRTMIN)
staticintrestart_signum=-1;
if (restart_signum==-1)
restart_signum=mono_threads_suspend_search_alternative_signal ();
returnrestart_signum;
#else
returnSIGXCPU;
#endif
}

Should we have some mechanism that the runtime could use to prevent users from installing handlers for these signals?

@tmds

tmds commented Jun 21, 2021

Copy link
Copy Markdown
MemberAuthor

what's the expected behavior if users set up a handler for a signal that the runtime uses internally.

By default the original handler gets called.
That should make most signals work. Some won't work, because they need some more specific handling.

Should we have some mechanism that the runtime could use to prevent users from installing handlers for these signals?

I don't have a strong opinion if we should forbid certain values.
When using raw signal numbers, the user should know what he is doing.

@tmds

tmds commented Jul 5, 2021

Copy link
Copy Markdown
MemberAuthor

CI fail is unrelated.

@stephentoub
stephentoub merged commit a25bece into dotnet:mainJul 8, 2021
@stephentoub

Copy link
Copy Markdown
Member

Thanks!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@tmds@andyleejordan@alexrp@lambdageek@stephentoub@lewing@davidfowl@jkotas@JamesWTruher@janvorli@danmoseley@ViktorHofer