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

Can ProcessBuilder be used to run sendctrlc.exe? #55

Closed
segrey opened this issue Aug 17, 2018 · 10 comments
Closed

Can ProcessBuilder be used to run sendctrlc.exe? #55

segrey opened this issue Aug 17, 2018 · 10 comments

Comments

@segrey
Copy link
Contributor

segrey commented Aug 17, 2018

Currently, sendctrlc.exe is spawned in winp.cpp.
Is it possible to use java.lang.ProcessBuilder to spawn sendctrlc.exe and, thus, simplify code?
If no, it'd be great to write a comment in code explaining decision.

@segrey
Copy link
Contributor Author

segrey commented Aug 27, 2018

@oleg-nenashev @stephanreiter Any thoughts? I would appreciate your feedback.

@oleg-nenashev
Copy link
Member

Sorry, missed the previous notification.
IIRC there was a discussion about that in the pull request, will try to find it.
Native code is not considered as API, so we can always change it without breaking changes.

@stephanreiter
Copy link

Using ProcessBuild is no problem. Good idea! Less native code ...
SendCtrlC ended up in native because it started there with some implementation that didn't use a separate EXE.

@segrey
Copy link
Contributor Author

segrey commented Aug 27, 2018

@oleg-nenashev Thanks. I meant something like

  public static boolean sendCtrlC(int pid) {
    Process process = new ProcessBuilder("sendctrlc.exe", pid).start();
    return waitFor(process, 5000) != null;
  }

  @Nullable
  private static Integer waitFor(@NotNull Process process, int timeoutMillis) {
    long stop = System.currentTimeMillis() + timeoutMillis;
    while (System.currentTimeMillis() < stop) {
      try {
        Thread.sleep(100);
        return process.exitValue();
      }
      catch (Exception ignore) {
      }
    }
    return null;
  }

This way, ProcessBuilder is not exposed in winp's API, so it can be changed without breaking changes too.

@stephanreiter
Copy link

Will that terminate the sendctrl.exe process after 5secs? That shouldn't be left lingering.

@stephanreiter
Copy link

Same should be (made) true for the native code! :)

@segrey
Copy link
Contributor Author

segrey commented Aug 27, 2018

Oops, process.destroy() is missing in waitFor. Seems it will terminate sendctrlc.exe.
Wondering, have you encountered any pitfalls in ProcessBuilder's behavior of spawning processes that leads to some problems?
Another question: is it possible that running sendctrlc.exe <pid> will also terminate a parent process of a process with ? AFAIU, a child process inherits console of its parent process, so if ctrl+c event is sent to a console, parent process will receive it too, right?

@stephanreiter
Copy link

I don't know much about ProcessBuilder anymore. :)

And sendctrlc.exe will first detach from its console, then attach to the process to avoid what you describe w.r.t. the parent process.

FreeConsole(); if (!AttachConsole(pid)) { return GetLastError(); }

@segrey
Copy link
Contributor Author

segrey commented Aug 27, 2018

@stephanreiter Sorry for not being clear. Yes, sendctrlc.exe deattaches from its initial console to be able to attach to a console of the specified process. But what if several processes are already attached to the console, sendctrlc.exe tries to attach to? For example, parent_pid and pid are attached to the same console and sendctrlc.exe pid is spawned. Looks like parent_pid will receive CtrlC event as well. Honestly, I haven't managed to reproduce such case, just wondering if it's possible.

@segrey segrey closed this as completed Aug 27, 2018
@stephanreiter
Copy link

Ah, now I understand. Well, then the Ctrl+C will go to the parent as well. I guess that should be documented to make it obvious.

segrey added a commit to segrey/winp that referenced this issue Nov 22, 2018
…nkinsci#60, jenkinsci#55)

Additionally, diagnostics is improved by capturing output stream.
oleg-nenashev added a commit that referenced this issue Dec 8, 2018
run sendctrlc.exe with Java (no console window, simpler code) (#60, #55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants