1
\$\begingroup\$

I basically want to write a simple GUI program to sync to directories using the rsync command. The core part of the code is below. My main question is, do I need to have the waitFor method included? At present, I have left it out, because I would like to show the user the --progress responses as it would in terminal, not wait until the end. Is this a bad idea?

public void executeCommand(String src, String dest) {
        System.out.println("Starting shell");
    Process p;
    try {
                System.out.println("Creating process...");
        p = Runtime.getRuntime().exec("rsync --progress -va "+src +" "+ dest );
        //p.waitFor(); <- is this important?
                    System.out.println("Creating reader...");
        BufferedReader reader =
                        new BufferedReader(new InputStreamReader(p.getInputStream()));                       
                    String line = "";
                    System.out.println("Starting sync...");
        while ((line = reader.readLine())!= null) {             
                            System.out.println(line);                              
                            textAreaAppend(line);   
        }

    } catch (Exception e) {
        e.printStackTrace();
    }
}

   public void textAreaAppend(String line){
    textArea.append(line +"\n");
}
\$\endgroup\$
1
\$\begingroup\$

Process.waitFor will terminate the current thread until the process is terminated. Thus, at the place where you commented it out, it does not make any sense at all. You might consider putting a p.waitFor() after the loop, as to continue with your program only when the process has terminated.

Apart from that, sorry to say so, your code is a mess. You use a very outdated method to start the process, you do not read stderr (which may lead to blocking conditions on stdout), code format is all over the place, p is declared outside the try block without any reason, exception handling is not present.

To at least cover the basic necessities of reading both stdin and stderr, you should go for process builder and redirect stderr to stdout:

    ProcessBuilder builder = new ProcessBuilder(Arrays.asList("rsync", "--progress", "-va", src, dest));
    builder.redirectErrorStream(true);
    builder.redirectOutput(ProcessBuilder.Redirect.PIPE);
    Process p = builder.start();
\$\endgroup\$
  • \$\begingroup\$ Thanks @mtj, I really appreciate your constructive criticism. Do I understand correctly that with these redirects, the errorstream will be included in my reader? \$\endgroup\$ – Istha Feb 28 '17 at 9:32
  • \$\begingroup\$ Yes. The redirectErrorStream(true) call will stream the error to the same channel as stdout (so you only have to read one stream, not two) and the redirectOutput(PIPE) will supply this as a stream as used in p.getInputStream(). \$\endgroup\$ – mtj Feb 28 '17 at 9:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.