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

Stream feeder and consumer threads should be replaced with ProcessBuilder->inheritIO to allow child processes to die/be killed on Windows when hitting CTRL-C #59

Open
jjYBdx4IL opened this issue Jan 31, 2019 · 3 comments

Comments

@jjYBdx4IL
Copy link

jjYBdx4IL commented Jan 31, 2019

tbroyer/gwt-maven-plugin#110 (comment)

I played around with plexus-utils to find out why my own test case works, but plexus-utils doesn't. Answer: it seems to be related to the stream pumpers/feeders. When I replace Runtime.exec() with ProcessBuilder and use its inheritIO() instead, the child process goes away as expected when the parent process calls Process->destroy() on it. Specifically using Redirect.INHERIT on the child's STDIN fixes the issue.

Commandline:

/**
 * Executes the command.
 */
public Process execute()
    throws CommandLineException
{
    // TODO: Provided only for backward compat. with <= 1.4
    verifyShellState();

    Process process;

    // addEnvironment( "MAVEN_TEST_ENVAR", "MAVEN_TEST_ENVAR_VALUE" );

    String[] environment = getEnvironmentVariables();

    File workingDir = shell.getWorkingDirectory();

    ProcessBuilder pb = new ProcessBuilder(getCommandline());
    pb.directory(workingDir);
    pb.inheritIO();
    if (environment != null) {
        for (String s : environment) {
            String[] kv = s.split("=", 2);
            pb.environment().put(kv[0], kv[1]);
        }
    }
    
    try
    {
        if ( workingDir != null ) {
            if ( !workingDir.exists() )
            {
                throw new CommandLineException( "Working directory \"" + workingDir.getPath()
                    + "\" does not exist!" );
            }
            else if ( !workingDir.isDirectory() )
            {
                throw new CommandLineException( "Path \"" + workingDir.getPath()
                    + "\" does not specify a directory." );
            }
        }

        process = pb.start();
    }
    catch ( IOException ex )
    {
        throw new CommandLineException( "Error while executing process.", ex );
    }

    return process;
}
@olamy
Copy link
Member

olamy commented Feb 1, 2019

sounds a good idea and to be tested!
could you make a pr with your proposal?

@jjYBdx4IL
Copy link
Author

While fixing gwt-maven-plugin, I found it easier to just use ProcessBuilder there. Do you think the command line exec stuff in plexus-utils is still necessary as of Java 7?

@michael-o
Copy link
Member

@jjYBdx4IL I think most not, espcially launching subshells is a mess.

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