diff -r 7c17199fa37d src/java.base/share/classes/java/util/spi/ToolProvider.java --- a/src/java.base/share/classes/java/util/spi/ToolProvider.java Fri Feb 15 08:21:08 2019 -0500 +++ b/src/java.base/share/classes/java/util/spi/ToolProvider.java Sat Feb 16 00:57:54 2019 +0100 @@ -126,8 +126,9 @@ default int run(PrintStream out, PrintStream err, String... args) { Objects.requireNonNull(out); Objects.requireNonNull(err); + Objects.requireNonNull(args); for (String arg : args) { - Objects.requireNonNull(args); + Objects.requireNonNull(arg); } PrintWriter outWriter = new PrintWriter(out); diff -r 7c17199fa37d test/jdk/java/util/spi/ToolProviderTest.java --- a/test/jdk/java/util/spi/ToolProviderTest.java Fri Feb 15 08:21:08 2019 -0500 +++ b/test/jdk/java/util/spi/ToolProviderTest.java Sat Feb 16 00:57:54 2019 +0100 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -46,16 +46,38 @@ void run() throws Exception { initServices(); + testNullArgs(); + System.out.println("test without security manager present:"); - test(); + testSecurityManager(); System.setSecurityManager(new SecurityManager()); System.out.println("test with security manager present:"); - test(); + testSecurityManager(); } - private void test() throws Exception { + private void testNullArgs() throws Exception { + ToolProvider testProvider = ToolProvider.findFirst("test").get(); + + // args array null check + try { + testProvider.run(System.out, System.err, (String[]) null); + throw new Error("NullPointerException not thrown"); + } catch (NullPointerException e) { + // expected + } + + // args array elements null check + try { + testProvider.run(System.out, System.err, (String) null); + throw new Error("NullPointerException not thrown"); + } catch (NullPointerException e) { + // expected + } + } + + private void testSecurityManager() throws Exception { ToolProvider testProvider = ToolProvider.findFirst("test").get(); int rc = testProvider.run(System.out, System.err, "hello test"); if (rc != 0) { @@ -92,7 +114,7 @@ // system property System.getProperty("java.home"); if (haveSecurityManager) { - throw new Error("exception exception not thrown"); + throw new Error("SecurityException not thrown"); } } catch (SecurityException e) { if (!haveSecurityManager) {

I still don't see why it is necessary to specify this behavior. -- Jon On 2/16/19 4:45 PM, Philipp Kunz wrote:

Hi Jon,

On Sat, 2019-02-16 at 13:44 -0800, Jonathan Gibbons wrote:

...

On 2/16/19 12:20 AM, Philipp Kunz wrote:

...

I'm also wondering about the call to flush in run(PrintStream out, PrintStream err, String... args). It looks like the intention was to flush the wrapping PrintWriter. That is not possible without also flushing the underlying PrintStream. BufferedWriter.flushBuffer would be a more sensible method to call but is not accessible. The effect is actually, that the call to PrintWriter.flush will also call flush of the underlying PrintStream. Should that be documented more explicitly, for example:

Philipp,

I don't see that it needs to be specified. It is a reasonable presumption that everything written by the tool is propagated to the streams passed into the run method. How that is achieved is an implementation detail.

I absolutely agree so far. My point, however, was a different one. In addition to flushing the buffers created by run(PrintStream, PrintStream, String...)'s PrintWriters to the PrintStreams passed to that method, flushing the PrintWriters propagates to the underlying PrintStreams and flushes them as well. In other words, whatever PrintStreams passed into run(PrintStream, PrintStream, String...) are flushed.

...

If you were to modify the spec, it would at most be an implementation detail, and should appear in an @implNote.

I agree that it should go into an @implNote. I also suggested it that way, below an existing @implNote.

...

-- Jon

Additionally, run(PrintStream, PrintStream, String...) is only a default method and may be replaced in ToolProvider implementations. Therefore we don't know if these buffers will actually be flushed and I replaced "are" with "may be" before "flushed" in the comment. The same consideration also applies to run(PrintWriter, PrintWriter, String...).

diff -r 31e3aa9c0c71 src/java.base/share/classes/java/util/spi/ToolProvider.java --- a/src/java.base/share/classes/java/util/spi/ToolProvider.java Sat Feb 16 11:40:34 2019 +0900 +++ b/src/java.base/share/classes/java/util/spi/ToolProvider.java Sun Feb 17 01:30:30 2019 +0100 @@ -75,6 +75,8 @@       * @apiNote The interpretation of the arguments will be specific to       * each tool.       * +     * @implNote Both {@code <mailto:{@code> out} and {@code <mailto:{@code> err} streams may be flushed. +     *       * @param out a stream to which "expected" output should be written       *       * @param err a stream to which any error messages should be written @@ -107,6 +109,8 @@       * @implNote This implementation wraps the {@code <mailto:{@code> out} and {@code <mailto:{@code> err}       * streams within {@link <mailto:{@link> PrintWriter}s, and then calls       * {@link <mailto:{@link> #run(PrintWriter, PrintWriter, String[])}. +     * Both {@code <mailto:{@code> out} and {@code <mailto:{@code> err} streams may be flushed before the method +     * returns as a side-effect of flushing the wrapping {@link <mailto:{@link> PrintWriter}s.       *       * @param out a stream to which "expected" output should be written       *

Regards, Philipp