Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with signal(SIGUSR1) #1888

Open
AlexDaniel opened this issue May 31, 2018 · 7 comments
Open

Issues with signal(SIGUSR1) #1888

AlexDaniel opened this issue May 31, 2018 · 7 comments
Labels
regression Issue did not exist previously signals tests needed Issue is generally resolved but tests were not written yet

Comments

@AlexDaniel
Copy link
Contributor

Code:

use NativeCall;
sub kill(int32, int32) is native {*};

my $x = signal(SIGUSR1).Channel;
kill $*PID, 10;
sleep 0.5;
say $x.poll

Output «2015.12⌁2018.03»:

SIGUSR1

Result «2018.04.1,2018.05»:

 «exit signal = 10»

Output HEAD(4109ce3):

10

Basically, from 2015.12 to 2018.03 it was able to handle signals correctly. Then in 2018.04 and 2018.05 it was completely broken (wasn't catching the signal at all). Currently on HEAD it works again, but the returned value from the supply is now numeric. I don't know if it should be this way.

There are two reasons for this ticket:

  1. First of all, we should have tests for that code.
  2. Secondly, given that the last two releases (including a Star release) come with a bug, and given that no workaround is known at this moment, we should discuss what should be done about this.
@AlexDaniel AlexDaniel added tests needed Issue is generally resolved but tests were not written yet regression Issue did not exist previously labels May 31, 2018
@AlexDaniel
Copy link
Contributor Author

AlexDaniel commented May 31, 2018

Good correction from lizmat++, the code to reproduce the problem only works on systems where SIGUSR1 is 10. You can replace 10 with the appropriate value, or send a signal using other means. The problem is that signal(SIGUSR1) on 2018.04 and 2018.05 is unable to catch a SIGUSR1 signal (and which way you use to send it is irrelevant).

AlexDaniel added a commit to Raku/whateverable that referenced this issue May 31, 2018
@AlexDaniel
Copy link
Contributor Author

AlexDaniel commented May 31, 2018

Interestingly, the workaround is to use signal(SIGBUS) – that works as signal(SIGUSR1) on 2018.04 and 2018.05 (on some platforms only). I rewrote the implementation-specific note in the docs, maybe that would help those struggling with this problem.

As far as I can see, we'll have to wait for 2018.06 release.

Meanwhile I had to do this in whateverable.

Please let me know what you think.

@jstuder-gh
Copy link
Contributor

Unfortunately this is due to the signal changes I introduced.

MoarVM used to translate the signums prior to installing the sighandler so if the user entered SIGUSR1 (30) from Rakudo, it would be translated to 10 internally. The getsignals PR changed that with the expectation that the Rakudo values would match the host system after the Rakudo getsignals PR was merged.

Unfortunately there was a fairly large difference in when the MoarVM PR got merged in to when the NQP and Rakudo PRs got merged.

I really should have better communicated the potential issues that could occur if one part was merged without the others, but I didn't. In fact, I had forgotten it myself, thinking instead that it was a benign change and that the op would just go unused until the others got merged. That was incorrect and I'm not sure why I didn't remember the potential for harm. As a result, I didn't push very hard for the NQP and Rakudo components to get merged in.

@AlexDaniel, as you mentioned, this should not be affecting MacOSX and BSD systems as their signal values match the constant values set in Rakudo pre-getsignals merge. Also, even on GNU/Linux, signals with explicit values in the POSIX spec also match (SIGINT, SIGQUIT, SIGKILL, SIGSEGV, etc).

I suppose that it would be a good idea to introduce patch releases for 2018.04 and 2018.05, although I don't know how much trouble that would be from the packaging and distrobution side. The NQP and Rakudo PRs merge cleanly onto the 2018.04.1 and 2018.05.

I don't know why the output of pre-2018.04 and HEAD differ. I can look into that more later today.

@AlexDaniel
Copy link
Contributor Author

I don't know why the output of pre-2018.04 and HEAD differ. I can look into that more later today.

Yeah, it's probably better to figure this out before we commit to anything.

But either way I can't really tell if point releases are justified. Only some signals are broken, and only on some platforms. That's very unfortunate but yet I'm the only person so far who noticed, I think. Also, signal values were originally broken until very recently, and it is somewhat luck that signal handlers were not affected.

Ping @nxadm @stmuk @robertlemmen, let me know what you think. I can cut point releases no problem, if that is really needed.

@nxadm
Copy link
Collaborator

nxadm commented May 31, 2018

Thx @AlexDaniel for being consulted.

With the monthly Rakudo release schedule in mind, I don't see the point for dot releases for older Rakudos. You just move to the fixed release if necessary (or automatically if you're using my repos).

I do find dot releases useful, however, for the last released Rakudo. It usefulness depends on the severity of the newly introduced bug and the time until the next release. Depending on real life scenarios of people being bitten with this bug, a dot 2015.05.2 release may be useful if the needed fix does not take the amount of time that will result in a release just a few days before the scheduled release.

C.

PS: Linux distributions that package rakudo may backport the changes, but that is an other use-case than rakudo-pkg that focuses on being up-to-date with the last release.

jstuder-gh added a commit to jstuder-gh/rakudo that referenced this issue Jun 3, 2018
On the recent modification of the signal sub, there was a subtle change
to the form of the signum emitted by the signal handler. Pre-change it
emitted the signum in the form of the corresponding Signal enum and
post-change it emits the signum as an Int. Changing it so that it will
one again emit the Signal enum as it is more useful in varying contexts.
AlexDaniel++ for discovering this.

See [Github Issue 1888](rakudo#1888).
@jstuder-gh
Copy link
Contributor

I've submitted a PR that addresses the difference in output between pre-2018.04 and HEAD.

Pre-2018.04 was emitting the Signal enumeration while afterwards was just emitting the signum as an Int.

@stmuk
Copy link
Contributor

stmuk commented Jun 3, 2018

I pretty much agree with @nxadm and @AlexDaniel Given limited resources, the minor nature of this particular bug and a work around I don't think it matters too much. Maybe a low severity https://alerts.perl6.org/ might be worth it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Issue did not exist previously signals tests needed Issue is generally resolved but tests were not written yet
Projects
None yet
Development

No branches or pull requests

5 participants