<AWT Dev> RFR: 8262161 Refactor manual I/O stream copying to new convinient methods in java.desktop

[Andrey Turbanov-2]

Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
1. java.io.InputStream#readAllBytes
2. java.io.InputStream#transferTo
3. java.nio.file.Files#copy

Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

-------------

Commit messages:
 - [PATCH] Refactor manual I/O stream copying to new convenient methods in java.desktop
 - [PATCH] Refactor I/O stream copying to use java.io.InputStream.transferTo in java.desktop

Changes: https://git.openjdk.java.net/jdk/pull/1856/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1856&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262161
  Stats: 134 lines in 11 files changed: 3 ins; 102 del; 29 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1856.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1856/head:pull/1856

PR: https://git.openjdk.java.net/jdk/pull/1856

[Sergey Bylokhov-2]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy
>
> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

The changes look fine, I'll run the tests.

src/java.desktop/windows/classes/sun/print/Win32PrintJob.java line 435:

> 433:             if (mDestination != null) { // if destination attribute is set
> 434:                 try {
> 435:                     Files.copy(instream, Path.of(mDestination));

Looks like the new code unlike the old one will throw an exception if the file is exists already. WIll it affect the printing functionality?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Sergey Bylokhov-2]

On Mon, 22 Feb 2021 18:38:36 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
>> 1. java.io.InputStream#readAllBytes
>> 2. java.io.InputStream#transferTo
>> 3. java.nio.file.Files#copy
>>
>> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272
>
> The changes look fine, I'll run the tests.

I have filed https://bugs.openjdk.java.net/browse/JDK-8262161

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Andrey Turbanov-2]

On Sun, 3 Jan 2021 03:08:31 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
>> 1. java.io.InputStream#readAllBytes
>> 2. java.io.InputStream#transferTo
>> 3. java.nio.file.Files#copy
>>
>> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272
>
> src/java.desktop/windows/classes/sun/print/Win32PrintJob.java line 435:
>
>> 433:             if (mDestination != null) { // if destination attribute is set
>> 434:                 try {
>> 435:                     Files.copy(instream, Path.of(mDestination));
>
> Looks like the new code unlike the old one will throw an exception if the file is exists already. WIll it affect the printing functionality?

good catch! Added `StandardCopyOption.REPLACE_EXISTING` to preserve behavior.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Prasanta Sadhukhan-2]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy
>
> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

src/java.desktop/windows/classes/sun/print/Win32PrintJob.java line 436:

> 434:             if (mDestination != null) { // if destination attribute is set
> 435:                 try {
> 436:                     Files.copy(instream, Path.of(mDestination), StandardCopyOption.REPLACE_EXISTING);

Don't we need to close the instream in finally block?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Andrey Turbanov-2]

On Tue, 23 Feb 2021 15:47:20 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
>> 1. java.io.InputStream#readAllBytes
>> 2. java.io.InputStream#transferTo
>> 3. java.nio.file.Files#copy
>>
>> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272
>
> src/java.desktop/windows/classes/sun/print/Win32PrintJob.java line 436:
>
>> 434:             if (mDestination != null) { // if destination attribute is set
>> 435:                 try {
>> 436:                     Files.copy(instream, Path.of(mDestination), StandardCopyOption.REPLACE_EXISTING);
>
> Don't we need to close the instream in finally block?

It will be closed in `notifyEvent(PrintJobEvent.JOB_FAILED);` -> `closeDataStreams` in case of IOException.
And there shouldn't be any other kinds of exceptions.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Phil Race]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy
>
> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

src/java.desktop/unix/classes/sun/print/UnixPrintJob.java line 601:

> 599:             try (BufferedInputStream bin = new BufferedInputStream(instream);
> 600:                  BufferedOutputStream bout = new BufferedOutputStream(output)) {
> 601:                 bin.transferTo(bout);

https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#transferTo(java.io.OutputStream)

This method does not close either stream.

---

So this doesn't look right.

src/java.desktop/share/classes/com/sun/media/sound/DLSSoundbank.java line 990:

> 988:             AudioInputStream stream = AudioSystem.getAudioInputStream(
> 989:                     audioformat, (AudioInputStream)sample.getData());
> 990:             stream.transferTo(data_chunk);

Are all these audio streams buffered ? transferTo docs don't say anything in terms of guarantees of what API they call. If it is unbuffered and it just calls read(byte) over and over it would be really slow.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Sergey Bylokhov-2]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy
>
> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

The direct replacement of the old API by the new one looks fine. The tests are green.

-------------

Marked as reviewed by serb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1856

[Sergey Bylokhov-2]

On Wed, 24 Feb 2021 20:48:29 GMT, Phil Race <[hidden email]> wrote:

>> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
>> 1. java.io.InputStream#readAllBytes
>> 2. java.io.InputStream#transferTo
>> 3. java.nio.file.Files#copy
>>
>> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272
>
> src/java.desktop/unix/classes/sun/print/UnixPrintJob.java line 601:
>
>> 599:             try (BufferedInputStream bin = new BufferedInputStream(instream);
>> 600:                  BufferedOutputStream bout = new BufferedOutputStream(output)) {
>> 601:                 bin.transferTo(bout);
>
> https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#transferTo(java.io.OutputStream)
>
> This method does not close either stream.
>
> ---
>
> So this doesn't look right.

The method itself does not close it, but the "try-with-res" around it should.

> src/java.desktop/share/classes/com/sun/media/sound/DLSSoundbank.java line 990:
>
>> 988:             AudioInputStream stream = AudioSystem.getAudioInputStream(
>> 989:                     audioformat, (AudioInputStream)sample.getData());
>> 990:             stream.transferTo(data_chunk);
>
> Are all these audio streams buffered ? transferTo docs don't say anything in terms of guarantees of what API they call. If it is unbuffered and it just calls read(byte) over and over it would be really slow.

It read the data in chunks of "8192" bytes.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Phil Race]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy
>
> Similar issue - https://bugs.openjdk.java.net/browse/JDK-8080272

Marked as reviewed by prr (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Phil Race]

On Fri, 26 Feb 2021 20:21:21 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> src/java.desktop/unix/classes/sun/print/UnixPrintJob.java line 601:
>>
>>> 599:             try (BufferedInputStream bin = new BufferedInputStream(instream);
>>> 600:                  BufferedOutputStream bout = new BufferedOutputStream(output)) {
>>> 601:                 bin.transferTo(bout);
>>
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#transferTo(java.io.OutputStream)
>>
>> This method does not close either stream.
>>
>> ---
>>
>> So this doesn't look right.
>
> The method itself does not close it, but the "try-with-res" around it should.

ah yes

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856

[Andrey Turbanov-2]

On Mon, 21 Dec 2020 07:54:17 GMT, Andrey Turbanov <[hidden email]> wrote:

> Cleanup code to use new handy methods in `java.io.InputStream`/`java.nio.file.Files` instead of manual stream copy:
> 1. java.io.InputStream#readAllBytes
> 2. java.io.InputStream#transferTo
> 3. java.nio.file.Files#copy

This pull request has now been integrated.

Changeset: 39b11138
Author:    Andrey Turbanov <[hidden email]>
Committer: Sergey Bylokhov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/39b11138
Stats:     134 lines in 11 files changed: 3 ins; 102 del; 29 mod

8262161: Refactor manual I/O stream copying in java.desktop to use new convenience APIs

Reviewed-by: serb, prr

-------------

PR: https://git.openjdk.java.net/jdk/pull/1856