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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

<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
Reply | Threaded
Open this post in threaded view
|

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

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
Reply | Threaded
Open this post in threaded view
|

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

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
Reply | Threaded
Open this post in threaded view
|

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

Andrey Turbanov-2
In reply to this post by Sergey Bylokhov-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
Reply | Threaded
Open this post in threaded view
|

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

Prasanta Sadhukhan-2
In reply to this post by 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
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

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
Reply | Threaded
Open this post in threaded view
|

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

Phil Race
In reply to this post by 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
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov-2
In reply to this post by 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
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov-2
In reply to this post by Phil Race
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
Reply | Threaded
Open this post in threaded view
|

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

Phil Race
In reply to this post by 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
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Phil Race
In reply to this post by Sergey Bylokhov-2
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
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> Integrated: 8262161: Refactor manual I/O stream copying in java.desktop to use new convenience APIs

Andrey Turbanov-2
In reply to this post by 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