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

Process.kill(:KILL, $$) doesn't work on Windows #5423

Closed
yamam opened this Issue Nov 6, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@yamam
Copy link

yamam commented Nov 6, 2018

Environment

jruby 9.2.1.0 (2.5.0) 2018-11-06 7b14404 Java HotSpot(TM) 64-Bit Server VM 25.191-b12 on 1.8.0_191-b12 +jit [mswin32-x86_64]

Expected Behavior

jruby-9.2.0.0/bin/jruby -e 'Process.kill(:KILL, $$)'
No error occurs.

Actual Behavior

jruby-9.2.1.0/bin/jruby -e 'Process.kill(:KILL, $$)'

The following error occurred.

NotImplementedError: this signal not yet implemented in windows
    kill at org/jruby/RubyProcess.java:1341
    kill at org/jruby/RubyProcess.java:1262
@ahorek

This comment has been minimized.

Copy link
Contributor

ahorek commented Nov 7, 2018

this is probably because SIGKILL 9 is indeed not implement on Windows, see
https://github.com/jnr/jnr-constants/pull/45/files#diff-20075114c23f4a8ded9c1c6fb57a358dL28
it looks like it was altered manually before, probably because of this problem.

I can confirm, that it works on MRI. We need to review what MRI do to handle this case.

@ahorek

This comment has been minimized.

Copy link
Contributor

ahorek commented Nov 7, 2018

it works on MRI because they did the same hack
https://github.com/ruby/ruby/blob/4444025d16ae1a586eee6a0ac9bdd09e33833f3c/include/ruby/win32.h#L484

Signal.list
=> MRI {"EXIT"=>0, "INT"=>2, "ILL"=>4, "ABRT"=>22, "FPE"=>8, "KILL"=>9, "SEGV"=>11, "TERM"=>15}
=> jruby {"SEGV"=>11, "ILL"=>4, "FPE"=>8, "TERM"=>15, "ABRT"=>22, "INT"=>2, "EXIT"=>0}

cc @headius

@enebo enebo added this to the JRuby 9.2.2.0 milestone Nov 7, 2018

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 7, 2018

@ahorek @headius not sure if jnr-posix should define KILL here for this. I think two simple platform checks in Signal.list (or the private method which builds list) and RubyProcess.parseSignalString() would solve this. It is specific to Ruby and not to posix in this case. This is a more brittle solution but it feels it is part of JRuby and not jnr-posix to me. A more elaborate solution would be to make our own Signal wrapper which includes KILL so then anything which uses Signal would just get that value. Less brittle but a little more refactoring.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 7, 2018

So this happened with the update of jnr-constants. When we generated all constants again, the manually-added values like SIGKILL on Windows got wiped out and reverted to their "fake" values. The logic in JRuby detects that SIGKILL is not defined on Windows and so doesn't define it for the Windows jnr-constants for Signal.

I don't think it's right to add to jnr-constants, since that is just supposed to reflect the actual constants on the system. We should fix in JRuby the same way MRI does.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 7, 2018

I just need to point out for posterity I kept saying jnr-posix but I meant jnr-constants since that was where the value was removed from... :)

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 7, 2018

It turns out that we were defining KILL on Windows but using the bogus "fake" value of "10" from jnr-constants. I have pushed a PR that omits undefined signal values and forces Windows to always define kill as 9. I noticed that MRI also defined SIGINT=2 but that constant appears to exist on Windows and in jnr-constants.

headius added a commit to headius/jruby that referenced this issue Nov 7, 2018

@headius headius closed this in c9b74d8 Nov 7, 2018

headius added a commit that referenced this issue Nov 7, 2018

Merge pull request #5430 from headius/windows_kill
Clean up signal mapping and force KILL to 9 on Windows. Fix #5423.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.