Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

[trivial] Move SIGUSR1/SIGUSR2 to SIGRT for GC#3617

Merged
dlang-bot merged 3 commits intodlang:masterfrom
rv32ima:move-sigusr-to-sigrt
May 24, 2022
Merged

[trivial] Move SIGUSR1/SIGUSR2 to SIGRT for GC#3617
dlang-bot merged 3 commits intodlang:masterfrom
rv32ima:move-sigusr-to-sigrt

Conversation

@rv32ima
Copy link
Contributor

@rv32ima rv32ima commented Nov 9, 2021

This is a revival of @9il's issue (Bugzilla Issue 15939, PR #1565), but fixed up / rebased on master.

As well, with this migration to the SIGRT signals, we no longer have to modify the signals for Android, so that line has been nixed.

Here is my rationale for this change (copied from the now-dead PR):

Is there any reason why this hasn't been merged, other then a pending rebase? SIGUSR1/SIGUSR2 seems rather problematic to use for something as low-level as the runtime, and I could easily imagine how this could shoot a developer in the foot if they're not aware. In fact, I think I've actually been burnt by this exact problem, as a C library that I wrapped was using SIGUSR1/SIGUSR2, and it was causing a variety of seemingly random issues (and as a library developer, I don't believe that I should override global GC constants).

In place of SIGUSR1/SIGUSR2, it seems safer to use RT signals, as they're less frequently used (and typically, if you're using RT signals, you're probably aware of what the runtime uses) and won't lead to confusion among new developers.

As well, SIGRTMIN thru SIGRTMIN + 8 are guaranteed to exist from Linux kernel 2.2+ (see POSIX.1-2001, signal(7) man pages, etc), so this should not break backwards compatibility in a significant manner. LGTM.

This may result in silent breakage (for developers who already use SIGRTMIN / SIGRTMIN + 1) -- I believe a changelog entry is in order?

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 9, 2021

Thanks for your pull request and interest in making D better, @hatf0! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
15939 blocker GC.collect causes deadlock in multi-threaded environment

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3617"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Nov 9, 2021
@rv32ima rv32ima changed the title Move SIGUSR1/SIGUSR2 to SIGRT for GC [trivial] Move SIGUSR1/SIGUSR2 to SIGRT for GC Nov 9, 2021
@dlang-bot dlang-bot added the Trivial typos, formatting, comments label Nov 9, 2021
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Can this be tested? Also this should definitely have a changelog.

// thread_resumeHandler does nothing.
version (Android) thread_setGCSignals(SIGUSR2, SIGUSR1);
// This hack is no longer needed, as the GC signals now use SIGRTMIN / SIGRTMIN + 1
// version (Android) thread_setGCSignals(SIGUSR2, SIGUSR1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say remove this altogether, it'll probably just be confusing and irrelevant in the future.

Copy link
Member

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

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

This might also silently break applications that rely on SIGRT since they are signals described as application-defined by the POSIX.1-2001. I'm not against it although, since this can significantly improve threads performance, but just take special care of this if needed.

ljmf00
ljmf00 previously requested changes Nov 14, 2021
@ljmf00
Copy link
Member

ljmf00 commented Nov 14, 2021

This is a revival of @9il's issue (Bugzilla Issue 15939, PR #1565), but fixed up / rebased on master.

I also suggest to add co-authorship trailing in the commit body based on the From header of the patch you based on https://github.com/dlang/druntime/commit/e6c192742c14958c5de048f2b48f9edbb6278ad7.patch

Something like this in the end of the commit body:

Co-authored-by: name <name@example.com>

@RazvanN7
Copy link
Contributor

ping @hatf0

@rv32ima
Copy link
Contributor Author

rv32ima commented Nov 30, 2021

@thewilsonator

Can this be tested? Also this should definitely have a changelog.

What would an acceptable test for this look like? I was thinking along the lines of adding in a signal handler and just ensuring that the signal thrown is SIGRTMIN, but I'm a little stumped on what it'd look like.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Dec 7, 2021

I think we can just get away with adding a changelog entry.

@RazvanN7
Copy link
Contributor

@hatf0 please add a changelog entry.

@kinke
Copy link
Contributor

kinke commented May 20, 2022

I took the liberty of removing the obsolete Android comment, adding a changelog entry and rebasing. - LLVM's libFuzzer uses SIGUSR1 too: https://github.com/llvm/llvm-project/blob/main/llvm/docs/LibFuzzer.rst#resuming-merge

@ljmf00 ljmf00 dismissed their stale review May 20, 2022 15:44

I believe this is consensually reasonable and worth changing

@kinke kinke requested review from RazvanN7 May 23, 2022 22:01
@RazvanN7
Copy link
Contributor

Once dlang/dmd#14154 is merged, this should be green.

@RazvanN7 RazvanN7 closed this May 24, 2022
@RazvanN7 RazvanN7 reopened this May 24, 2022
@dlang-bot dlang-bot merged commit bdeee86 into dlang:master May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Fix Include reference to corresponding bugzilla issue Trivial typos, formatting, comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants