[🐛 Bug]: ExternalProcess cannot properly handle graceful process termination

[mykola-mokhnach]

What happened?

See appium/java-client#2238 for more details.

I've tried to debug the issue locally and it looks like the main culprit lies in the default java's Process.destroy implementation:

   private void destroy(boolean force) {
        switch (OperatingSystem.current()) {
            case LINUX:
            case AIX:
            case MACOS:
                this.lock.lock();

                try {
                    if (!this.hasExited) {
                        this.processHandle.destroyProcess(force);
                    }
                } finally {
                    this.lock.unlock();
                }

                try {
                    this.stdin.close();
                } catch (IOException var9) {
                }

                try {
                    this.stdout.close();
                } catch (IOException var8) {
                }

                try {
                    this.stderr.close();
                } catch (IOException var7) {
                }

                return;
            default:
                throw new AssertionError("Unsupported platform: " + OperatingSystem.current());
        }

As you can see there after it sends the signal for process termination it also always implicitly closes all process streams, which creates EPIPE errors mentioned in the above issue and also drops several recent log lines from the Appium Node.js process log. As a fix we should not close process streams unless the next waitFor method completes or we destroy the process forcefully.

My proposed fix for this issue would be to update the implementation of org.openqa.selenium.os.ExternalProcess class -> shutdown method. Instead of

        process.destroy();

cal something like (pseudocode)

    if (isWindows) {
       run(["taskkill.exe", "/pid", process.pid]);
    } else {
       run(["kill", "-2", process.pid]);
    }

How can we reproduce the issue?

Just run the code from https://github.com/appium/java-client/issues/2238, which starts and stop Appium process. You may use the `npm i appium` command to install Appium itself.

Relevant log output

uncaughtException: write EPIPE
Error: write EPIPE

Operating System

all

Selenium version

latest

What are the browser(s) and version(s) where you see this issue?

all

What are the browser driver(s) and version(s) where you see this issue?

all

Are you using Selenium Grid?

No response

[github-actions]

@mykola-mokhnach, thank you for creating this issue. We will troubleshoot it as soon as we can.

---

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

[joerg1985]

@mykola-mokhnach i think the best fix is to use the process.toHandle().destroy() here to avoid closing the streams too early.
Do you have any concerns about this?

[mykola-mokhnach]

@mykola-mokhnach i think the best fix is to use the process.toHandle().destroy() here to avoid closing the streams too early. Do you have any concerns about this?

It should work, we should also make sure the process is alive (e.g. has valid PID) before invoking this method

[joerg1985]

The javadoc does state it is safe to call this method in case a process is already terminated.

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed. Please open a new issue for related bugs.