Empty string parsing ntpq command result

Question

I'm parsing the result of executing this composite command

  ntpq -c peers | awk ' $0 ~ /^*/ {print $9}'

in order to obtain the offset of the active ntp server.

This is the java code used and executed periodically

 public Double getClockOffset() {
    Double localClockOffset = null;

    try {

        String[] cmd = {"/bin/sh", 
                        "-c", 
                        "ntpq -c peers | awk \' $0 ~ /^\\*/ {print $9}\'"};

        Process p = Runtime.getRuntime().exec(cmd);

        p.waitFor();

        BufferedReader buf = new BufferedReader(new InputStreamReader(p.getInputStream()));

        String line = buf.readLine();

        if (!StringUtils.isEmpty(line)) {
            localClockOffset = Double.parseDouble(line.trim());
        } else {
            // Log "NTP -> Empty line - No active servers - Unsynchronized"
        }
    } catch (Exception e) {
        // Log exception
    }

    return localClockOffset;
}

ntpq result example

>      remote           refid      st t when poll reach   delay   offset  jitter
> ==============================================================================
> *server001s1     .LOCL.           1 u   33   64  377    0.111   -0.017   0.011
> +server002s1     10.30.10.6       2 u   42   64  377    0.106   -0.006   0.027
> +server003s1     10.30.10.6       2 u   13   64  377    0.120   -0.009   0.016

Notice that awk searchs the first line beginnig with '*' and extracts its ninth column. In the example: -0.017

The problem is that sometimes I'm obtaining the no-active-servers log message - intended to appear when there is no server with '*'- while the execution of the command through the console returns a number.

I know that I'm not closing the BufferedReader in that code but is that the reason of this behaviour? A new instance is being created (and left open until garbage collecting) in each method invocation but I think that it shouldn't be the cause of this problem.

Answer 1

As pointed by Andrew Thompson, you shall try ProcessBuilder instead.

String[] cmd = {"/bin/sh", 
                        "-c", 
                        "ntpq -c peers | awk \' $0 ~ /^\\*/ {print $9}\'"};
ProcessBuilder pb = new ProcessBuilder(cmd);
pb.redirectErrorStream(true);

Process proc = pb.start();
BufferedReader buf = new BufferedReader(new 
InputStreamReader(proc.getInputStream()));
String line = null;
while ((line = buf.readLine()) != null) {
   localClockOffset = Double.parseDouble(line.trim());
   break;
}

proc.destroy();

Ref ProcessBuilder

Answer 2

Runtime.exec() simply invokes the ProcessBuilder inside it, like that:

public Process More ...exec(String[] cmdarray, String[] envp, File dir)
    throws IOException {
    return new ProcessBuilder(cmdarray)
        .environment(envp)
        .directory(dir)
        .start();
}

see OpenJDK Runtime.java

So there is nothing wrong with using it instead of the ProcessBuilder as is.

The problem is that you invoke:

p.waitFor();

before you obtained the InputStream.

Which means that the process will be already terminated, by the time you obtain the InputStream, and the output stream data might be or might not be available to you, depending on the OS buffering implementation nuances and precise timing of the operations.

So, if you move the waitFor() to the bottom, your code should start working more reliably.

Under Linux however you should normally be able to read the remaining data from the PIPE buffer, even after the writing process has ended.

And the UNIXProcess implementation in OpenJDK, actually makes an explicit use of that, and tries to drain the remaining data, once the process has exited, so that file descriptor can be reclaimed:

/** Called by the process reaper thread when the process exits. */
synchronized void processExited() {
    synchronized (closeLock) {
        try {
            InputStream in = this.in;
            // this stream is closed if and only if: in == null
            if (in != null) {
                byte[] stragglers = drainInputStream(in);
                in.close();
                this.in = (stragglers == null) ?
                    ProcessBuilder.NullInputStream.INSTANCE :
                    new ByteArrayInputStream(stragglers);
            }
        } catch (IOException ignored) {}
    }
}

And this seems to work reliable enough, at least in my tests, so it would be nice to know which specific version of Linux|Unix and JRE your are running.

Have you also considered the possibility of an application-level problem ? I.e. ntpq is not really guaranteed to always return a * row.

So, it would be nice to remove the awk part from your pipe, to see if there will be some output at all the times.

Another thing to note is that if one of your shell pipeline steps fails (e.g. the ntpq itself), you will also get an empty output, so you will have to track the STDERR as well (e.g. by merging it with STDOUT via the ProcessBuilder).

Sidenote

Doing waitFor before you start consuming the data, is a bad idea in any case, as if your external process will produce enough output to fill the pipe buffer, it will just hang waiting for someone to read it, which will never happen, as your Java process will be locked in waitFor at the same time.

Answer 3

Finally we have found the real problem.

I'm not gonna change the accepted anwser, I think that it's useful too but maybe someone can learn from our experience.

My java program is launched with a shell script. When we execute the script manually, ntpq command is found and invoked successfully. The problem arises when the software is fully deployed. In the final environment we've got a cron scheduled demon that keeps our program alive but PATH established by cron is different from the PATH that our profile has got assigned.

PATH used by cron:

.:/usr/bin:/bin

PATH that we got login for launching the script manually:

/usr/sbin:/usr/bin:/bin:/sbin:/usr/lib:/usr/lib64:/local/users/nor:
/usr/local/bin:/usr/local/lib:.

Usually ntpq is in

/usr/sbin/ntpq

After we found the key of our problem, I search StackOverflow and got this relevant question where the problem is better explained and solved.

How to get CRON to call in the correct PATHs