Details

    • Number of attachments :
      0

      Description

      Currently, in RubyProcess, we have the following code:

        // Windows does not support these functions, so we won't even try
        // This also matches Ruby behavior for JRUBY-2353.
        if (Platform.IS_WINDOWS) {
          return runtime.getNil();
        }
      

      Which is wrong, silently ignoring the call. We either should raise NotImplementedError, or actually implement kill() on Windows, via jnr.

        Issue Links

          Activity

          Hide
          Roger Pack added a comment -
          Show
          Roger Pack added a comment - https://github.com/jruby/jruby/pull/320 for the 'KILL' signal.
          Hide
          Roger Pack added a comment -

          ok I think those branches mimic MRI now (though waiting back for word from them on one question, I think I understand the topic now http://bugs.ruby-lang.org/issues/7082#change-29770).

          Unfortunately with this branch I think it may have revealed another, separate bug also preventing it from quite working right (but that can probably just be filed later as a separate bug).

          Basically, the following should raise, but doesn't yet for some reason (I believe the implementation of Process.kill is ok though):

          {{

          { >>a = IO.popen('ls', 'w') => #<IO:fd 7> >> pid = a.pid => 4944 >> Process.kill 0, pid => 1 >> a.close => nil >> Process.kill 0, pid => 1 # should have raised, but process must still be zombied, I suppose. }

          }}

          Show
          Roger Pack added a comment - ok I think those branches mimic MRI now (though waiting back for word from them on one question, I think I understand the topic now http://bugs.ruby-lang.org/issues/7082#change-29770 ). Unfortunately with this branch I think it may have revealed another, separate bug also preventing it from quite working right (but that can probably just be filed later as a separate bug). Basically, the following should raise, but doesn't yet for some reason (I believe the implementation of Process.kill is ok though): {{ { >>a = IO.popen('ls', 'w') => #<IO:fd 7> >> pid = a.pid => 4944 >> Process.kill 0, pid => 1 >> a.close => nil >> Process.kill 0, pid => 1 # should have raised, but process must still be zombied, I suppose. } }}
          Hide
          Roger Pack added a comment -

          Ok after the latest round of research, I have left the windows_kill branch with a more aggressive Process.kill 0.

          Reason being is that if I don't, and match exact parity with MRI, it reveals another bug (the one mentioned above), so it's a good "stopping point" until the issue behind that other bug can be first addressed. Also it matches parity with Jruby on Linux, which also reaises EPERM "too early" in the case of https://gist.github.com/3798801

          So basically I'm ok with that branch how it is now (functionality matches Linux, it works but doesn't quite match MRI yet). I'd be happy to edit them to cleanup whitespace, etc. just let me know how.

          If/when merged then I'll file another bug that it raises EPERM "too early" on windows and linux.

          Thanks.

          Show
          Roger Pack added a comment - Ok after the latest round of research, I have left the windows_kill branch with a more aggressive Process.kill 0. Reason being is that if I don't, and match exact parity with MRI, it reveals another bug (the one mentioned above), so it's a good "stopping point" until the issue behind that other bug can be first addressed. Also it matches parity with Jruby on Linux, which also reaises EPERM "too early" in the case of https://gist.github.com/3798801 So basically I'm ok with that branch how it is now (functionality matches Linux, it works but doesn't quite match MRI yet). I'd be happy to edit them to cleanup whitespace, etc. just let me know how. If/when merged then I'll file another bug that it raises EPERM "too early" on windows and linux. Thanks.
          Hide
          Roger Pack added a comment -

          this can be closed

          Show
          Roger Pack added a comment - this can be closed
          Hide
          Roger Pack added a comment -

          Looks like MRI does
          OpenProcess(PROCESS_TERMINATE
          then TerminateProcess for SIGKILL
          and then "doesn't implement" SIGTERM.

          So maybe it would be better to have SIGKILL do both, but SIGTERM only do the OpenProcess one, is that what you were suggesting? (patches to both MRI and Jruby might be in order, if that's the case).

          Show
          Roger Pack added a comment - Looks like MRI does OpenProcess(PROCESS_TERMINATE then TerminateProcess for SIGKILL and then "doesn't implement" SIGTERM. So maybe it would be better to have SIGKILL do both, but SIGTERM only do the OpenProcess one, is that what you were suggesting? (patches to both MRI and Jruby might be in order, if that's the case).

            People

            • Assignee:
              Charles Oliver Nutter
              Reporter:
              Vladimir Sizikov
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: