RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

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

RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Brian Burkhalter-2
https://bugs.openjdk.java.net/browse/JDK-8194133
http://cr.openjdk.java.net/~bpb/8194133/webrev.00/

Add jdk.internal.io.IOSupport with copy() methods for InputStream-to-OutputStream copying and modify some classes to use these new methods.

One thing that I noticed when looking at this is that in the fix for https://bugs.openjdk.java.net/browse/JDK-8193842, the Files.copy() method had a loop like

while ((n = read(…)) > 0)

whereas InputStream.transferTo() had

while((n = read(…)) >= 0)

which is to say that Files.copy() would terminate if there were an empty read() but transferTo() would not. The patch for 8193842 therefore possibly introduced a subtle behavioral change which no one noticed.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

forax
Hi Brian,
it's not clear to me that all internal usages should use IOSupport.copy and not InputStream.transferTo,
a specific InputStream implementation (like DataInputStream) may come with a more optimized version of transferTo.

It remember me Collections.sort()/List.sort(), at first it was decided that List.sort() should call Collections.sort() but after thinking a little bit more, Collections.sort() was re-written to call List.sort() because it may use faster implementation (like ArrayList.sort()).

cheers,
Rémi

----- Mail original -----
> De: "Brian Burkhalter" <[hidden email]>
> À: "core-libs-dev" <[hidden email]>
> Envoyé: Vendredi 22 Décembre 2017 22:51:17
> Objet: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

> https://bugs.openjdk.java.net/browse/JDK-8194133
> http://cr.openjdk.java.net/~bpb/8194133/webrev.00/
>
> Add jdk.internal.io.IOSupport with copy() methods for
> InputStream-to-OutputStream copying and modify some classes to use these new
> methods.
>
> One thing that I noticed when looking at this is that in the fix for
> https://bugs.openjdk.java.net/browse/JDK-8193842, the Files.copy() method had a
> loop like
>
> while ((n = read(…)) > 0)
>
> whereas InputStream.transferTo() had
>
> while((n = read(…)) >= 0)
>
> which is to say that Files.copy() would terminate if there were an empty read()
> but transferTo() would not. The patch for 8193842 therefore possibly introduced
> a subtle behavioral change which no one noticed.
>
> Thanks,
>
> Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Alan Bateman
In reply to this post by Brian Burkhalter-2
On 22/12/2017 21:51, Brian Burkhalter wrote:

> https://bugs.openjdk.java.net/browse/JDK-8194133
> http://cr.openjdk.java.net/~bpb/8194133/webrev.00/
>
> Add jdk.internal.io.IOSupport with copy() methods for InputStream-to-OutputStream copying and modify some classes to use these new methods.
>
> One thing that I noticed when looking at this is that in the fix for https://bugs.openjdk.java.net/browse/JDK-8193842, the Files.copy() method had a loop like
>
> while ((n = read(…)) > 0)
>
> whereas InputStream.transferTo() had
>
> while((n = read(…)) >= 0)
>
> which is to say that Files.copy() would terminate if there were an empty read() but transferTo() would not. The patch for 8193842 therefore possibly introduced a subtle behavioral change which no one noticed.
>
read(byte[]) is blocking so it should only return 0 if called with a
0-length array. So not clear to me that Files.copy methods needs to use
this.

For JrtPath then the code is specific to the jimage -> jimage case. I
think that can be left along too or just replaced with a better
implementation for jrtfs.

-Alan.


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Brian Burkhalter-2

On Jan 2, 2018, at 9:01 AM, Alan Bateman <[hidden email]> wrote:

>> One thing that I noticed when looking at this is that in the fix for https://bugs.openjdk.java.net/browse/JDK-8193842, the Files.copy() method had a loop like
>>
>> while ((n = read(…)) > 0)
>>
>> whereas InputStream.transferTo() had
>>
>> while((n = read(…)) >= 0)
>>
>> which is to say that Files.copy() would terminate if there were an empty read() but transferTo() would not. The patch for 8193842 therefore possibly introduced a subtle behavioral change which no one noticed.
>>
> read(byte[]) is blocking so it should only return 0 if called with a 0-length array.

Good point so “>” should be used.

> So not clear to me that Files.copy methods needs to use this.

So you are suggesting leaving the call to InputStream.transferTo() in preference to IOSupport.copy()?

> For JrtPath then the code is specific to the jimage -> jimage case. I think that can be left along too or just replaced with a better implementation for jrtfs.

It would simplify this patch a little to leave this part out.

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Alan Bateman


On 02/01/2018 23:43, Brian Burkhalter wrote:
> :
>
>> So not clear to me that Files.copy methods needs to use this.
>
> So you are suggesting leaving the call to InputStream.transferTo() in
> preference to IOSupport.copy()?
Yes, I think these two should use transferTo.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Brian Burkhalter-2
On Jan 3, 2018, at 3:23 AM, Alan Bateman <[hidden email]> wrote:

> On 02/01/2018 23:43, Brian Burkhalter wrote:
>> :
>>
>>> So not clear to me that Files.copy methods needs to use this.
>>
>> So you are suggesting leaving the call to InputStream.transferTo() in preference to IOSupport.copy()?
> Yes, I think these two should use transferTo.

I don’t think that transferTo() can be used in JrtPath right now as BUILD_JRTFS sets the compiler option “—release 8” and transferTo() was added in 9. So unless this can be changed to 9, then JrtPath would need either to remain unchanged or use IOSupport.copy().

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Alan Bateman


On 04/01/2018 01:48, Brian Burkhalter wrote:

> On Jan 3, 2018, at 3:23 AM, Alan Bateman <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>> On 02/01/2018 23:43, Brian Burkhalter wrote:
>>> :
>>>
>>>> So not clear to me that Files.copy methods needs to use this.
>>>
>>> So you are suggesting leaving the call to InputStream.transferTo()
>>> in preference to IOSupport.copy()?
>> Yes, I think these two should use transferTo.
>
> I don’t think that transferTo() can be used in JrtPath right now as
> BUILD_JRTFS sets the compiler option “—release 8” and transferTo() was
> added in 9. So unless this can be changed to 9, then JrtPath would
> need either to remain unchanged or use IOSupport.copy().
>
Sorry, when I said "these two should use transferTo" then I meant the
Files.copy(Path, OutputStream) and Files.copy(InputStream, Path)
methods, not the jrtfs code. You are right that jrtfs is complicated as
it is compiled and packaged into jrtfs.jar for use by tools running on
JDK 8. You'll have to leave that as is (it can't use IOSupport.copy or
any other internal API).

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Brian Burkhalter-2
On Jan 4, 2018, at 12:06 AM, Alan Bateman <[hidden email]> wrote:

>>>>> So not clear to me that Files.copy methods needs to use this.
>>>>
>>>> So you are suggesting leaving the call to InputStream.transferTo() in preference to IOSupport.copy()?
>>> Yes, I think these two should use transferTo.
>>
>> I don’t think that transferTo() can be used in JrtPath right now as BUILD_JRTFS sets the compiler option “—release 8” and transferTo() was added in 9. So unless this can be changed to 9, then JrtPath would need either to remain unchanged or use IOSupport.copy().
>>
> Sorry, when I said "these two should use transferTo" then I meant the Files.copy(Path, OutputStream) and Files.copy(InputStream, Path) methods, not the jrtfs code. You are right that jrtfs is complicated as it is compiled and packaged into jrtfs.jar for use by tools running on JDK 8. You'll have to leave that as is (it can't use IOSupport.copy or any other internal API).

Patch updated per this e-mail thread:

http://cr.openjdk.java.net/~bpb/8194133/webrev.01/

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Alan Bateman
On 04/01/2018 17:18, Brian Burkhalter wrote:
>
> Patch updated per this e-mail thread:
>
> http://cr.openjdk.java.net/~bpb/8194133/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ebpb/8194133/webrev.01/>
>
Sorry for prolonging this one but is sun.net.NetworkServer used by
anything? I suspect it's a left over from early JDK releases and can be
deleted.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193842: Refactor InputStream-to-OutputStream copy into a utility method

Brian Burkhalter-2
Indeed it looks like NetworkServer could be removed. Question is what else in that package could be removed? Could be a separate issue.

On Jan 5, 2018, at 3:27 AM, Alan Bateman <[hidden email]> wrote:

> Sorry for prolonging this one but is sun.net.NetworkServer used by anything? I suspect it's a left over from early JDK releases and can be deleted.