Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Implement PosixSignal API#54136

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
stephentoub merged 65 commits intodotnet:mainfromtmds:posix_signal
Jul 8, 2021
Merged

Implement PosixSignal API#54136

stephentoub merged 65 commits intodotnet:mainfromtmds:posix_signal
Jul 8, 2021

Conversation

@tmds
Copy link
Member

Implements#50527.

I still need to add tests.

Can you do a first review of the implementation?

cc@janvorli@jkotas@stephentoub@davidfowl@bartonjs

davidfowl, andyleejordan, alexrp, and eriawan reacted with heart emoji
@ghost
Copy link

Note regarding thenew-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.

Copy link
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 theref file, I don't know if there is an automated way.

Copy link
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 inhttps://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
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
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 thePosixSignal bits.
I'm surprised to seeinternal instead ofprivate on thePosixSignalRegistration constructor.

@andyleejordan
Copy link
Member

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

tmds reacted with heart emojialexrp reacted with rocket emoji

@tmds
Copy link
MemberAuthor

The implementation has changed a bit since the initial PR.

  • PositivePosixSignal values can now be used as raw signal numbers.
  • Terminal configuration onSIGCONT/SIGCHLD can be canceled, as requested by@alexrp.

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

It handles specifically what is cancelable by the runtime usingPosixSignal API:

  • Not terminating forSIGTERM/SIGINT/SIGQUIT
  • Skipping terminal configuration forSIGCHLD/SIGCONT.

I'll look at writing some tests next week.

alexrp reacted with thumbs up emoji

@alexrp
Copy link
Contributor

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

@lambdageek
Copy link
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 usepthread_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
Copy link
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.

lambdageek reacted with thumbs up emoji

@tmds
Copy link
MemberAuthor

tmds commentedJul 5, 2021

CI fail is unrelated.

@stephentoubstephentoub merged commita25bece intodotnet:mainJul 8, 2021
@stephentoub
Copy link
Member

Thanks!

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

Reviewers

@lewinglewinglewing left review comments

@davidfowldavidfowldavidfowl left review comments

@lambdageeklambdageeklambdageek left review comments

@jkotasjkotasjkotas left review comments

@janvorlijanvorlijanvorli left review comments

@stephentoubstephentoubstephentoub approved these changes

+2 more reviewers

@alexrpalexrpalexrp left review comments

@JamesWTruherJamesWTruherJamesWTruher left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@tmds@andyleejordan@alexrp@lambdageek@stephentoub@lewing@davidfowl@jkotas@JamesWTruher@janvorli@ViktorHofer
[8]ページ先頭
©2009-2025 Movatter.jp