Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 GMT, Roger Riggs wrote:
>> Methods are added to java.lang.Process to read and write characters and
>> lines from and to a spawned Process.
>> The Charset used to encode and decode characters to bytes can be specified
>> or use the
>> operating system native encoding as is available from the "native.encoding"
>> system property.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Described charset mapping of malformed chars in outputWriter
> Repeated calls to inputReader, errorReader, and outputWriter now return
> the same instance
> and check for inconsistent charset argument
> Added warnings about concurrently use of input/output streams and
> readers/writers.
src/java.base/share/classes/java/lang/Process.java line 105:
> 103: // Readers and Writers created for this process; so repeated calls
> return the same object
> 104: // All updates must be done while synchronized on this Process.
> 105: private BufferedWriter outputWriter = null;
No need to explicitly initialise all these fields to null.
src/java.base/share/classes/java/lang/Process.java line 131:
> 129: * Use {@link #getOutputStream} and {@link #outputWriter} with
> extreme care.
> 130: * Output to the {@code BufferedWriter} may be held in the buffer
> until
> 131: * {@linkplain BufferedWriter#flush flush} is called.
I think this will need a bit of wordsmithing to make it clear that its the
usage of both streams for the same Process requires great care (as it stands,
it reads like the usage of either method is dangerous).
src/java.base/share/classes/java/lang/Process.java line 207:
> 205: * {@link Charset} named by the {@code native.encoding}
> 206: * system property or the {@link Charset#defaultCharset()} if the
> 207: * {@code native.encoding} is not supported.
"if the native.encoding is not supported". I think this needs an adjustment to
make it clear that the value of the property is not a valid charset.
src/java.base/share/classes/java/lang/Process.java line 425:
> 423: } else {
> 424: if (!outputCharset.equals(charset))
> 425: throw new IllegalArgumentException("BufferedWriter
> was created with charset: " + outputCharset);
I'm not sure that IAE is the right exception here, I think it's closer to
IllegalStateException because the first usage of outputWriter has the side
effect of setting the charset for the Process's writer. Another option here is
to just put it into the "unpredictable" bucket that is using getOutputStream
and outputWriter at the same time. In that case, it could just return a new
BufferedWriter when it doesn't match the charset of the first usage.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Thu, 27 May 2021 06:53:18 GMT, Alan Bateman wrote: > > Process is abstract. Is there any use for these new methods to be > > overridden? > > Perhaps they should be final. > > It's not clear to me that it is useful to extend Process outside of the JDK. > Testing, wrapping, ...? It feels like this class wants to be sealed. Maybe we > should look at deprecating the no-arg constructor, like Joe did recently with > AccessibleObject. There are subclasses, even in the test suite, see ProcessTools, so its too late to seal it; It would be a compatibility issue. We can look at deprecation and strength reduction but in the current time frame (RPD1) I'd suggest making the new methods final. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Wed, 26 May 2021 17:58:06 GMT, Roger Riggs wrote: > Process is abstract. Is there any use for these new methods to be overridden? > Perhaps they should be final. It's not clear to me that it is useful to extend Process outside of the JDK. Testing, wrapping, ...? It feels like this class wants to be sealed. Maybe we should look at deprecating the no-arg constructor, like Joe did recently with AccessibleObject. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 GMT, Roger Riggs wrote:
>> Methods are added to java.lang.Process to read and write characters and
>> lines from and to a spawned Process.
>> The Charset used to encode and decode characters to bytes can be specified
>> or use the
>> operating system native encoding as is available from the "native.encoding"
>> system property.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Described charset mapping of malformed chars in outputWriter
> Repeated calls to inputReader, errorReader, and outputWriter now return
> the same instance
> and check for inconsistent charset argument
> Added warnings about concurrently use of input/output streams and
> readers/writers.
src/java.base/share/classes/java/lang/Process.java line 254:
> 252: * {@code
> 253: * return new BufferedReader(new
> InputStreamReader(getInputStream(), charset));
> 254: * }
Does not seem to be equivalent any longer. Also, should it describe the
behavior that it rejects the different Charset after the first invocation?
src/java.base/share/classes/java/lang/Process.java line 326:
> 324: * {@code
> 325: * return new BufferedReader(new
> InputStreamReader(getErrorStream(), charset));
> 326: * }
Same comment in `inputReader(Charset)` stands here too.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 GMT, Roger Riggs wrote: >> Methods are added to java.lang.Process to read and write characters and >> lines from and to a spawned Process. >> The Charset used to encode and decode characters to bytes can be specified >> or use the >> operating system native encoding as is available from the "native.encoding" >> system property. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Described charset mapping of malformed chars in outputWriter > Repeated calls to inputReader, errorReader, and outputWriter now return > the same instance > and check for inconsistent charset argument > Added warnings about concurrently use of input/output streams and > readers/writers. Process is abstract. Is there any use for these new methods to be overridden? Perhaps they should be final. The suggestion in the CSR was to document them using @ implSpec, to acknowledge that the subclass can do something different. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
> Methods are added to java.lang.Process to read and write characters and lines > from and to a spawned Process. > The Charset used to encode and decode characters to bytes can be specified or > use the > operating system native encoding as is available from the "native.encoding" > system property. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Described charset mapping of malformed chars in outputWriter Repeated calls to inputReader, errorReader, and outputWriter now return the same instance and check for inconsistent charset argument Added warnings about concurrently use of input/output streams and readers/writers. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/7724b82c..a5ed7b73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=02-03 Stats: 161 lines in 2 files changed: 131 ins; 0 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/4134.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4134/head:pull/4134 PR: https://git.openjdk.java.net/jdk/pull/4134
