Updating existing JDK code to use InputStream.transferTo()

[Patrick Reinhart]

Hi Pavel,

On 14.05.2015 01:28, Pavel Rappo wrote:
>>> Let me start then.
>>>
>>> 1. I've seen several cases where current behaviour
>>>
>>>      while ((n = in.read(buffer)) > 0)
>>>                                  ~~~
>>> has been changed to
>>>
>>>      while ((read = this.read(buffer, 0, TRANSFER_BUFFER_SIZE)) >= 0)
>>>                                                                 ~~~
>>>
>>> Those things jump out at you, but don't seem to be harmful, since the only case
>>> where:
>>>
>>>      java.io.InputStream.read(byte[], int off, int len)
>>>      java.io.InputStream#read(byte[])
>>>
>>> may return 0, is when len == 0. And it's never the case here. Unless, of course,
>>> some misbehaving implementation of InputStream is used.
>> Do you think we should change our general behaviour to  "> -1" instead of ">=0" ?
> (Currently being discussed on core-libs-dev)
I saw that...
>>> 2. jdk/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/utils/JavaUtils.java
>>>
>>> Some of the refactored methods there seem to be unused:
>>>
>>>      getBytesFromFile(String)
>>>      writeBytesToFilename(String, byte[])
>>>
>>> Should we remove them instead?
>> When it's no longer used, it makes no sense to have this method I would say. Due to the edit process has took
>> place within a simple editor, I simply overlooked that fact.
> No problem. In fact I will need to check this with my colleagues to make sure we
> can safely do this. Otherwise if this thing, say, is regularly updated from some
> Apache project, they might bring it (and/or dependencies on it) the next time
> they update. This doesn't sound right to me. Thus we have to be super careful
> even with the update you've proposed.
Or we leave that out in those special cases where we use "external" sources
>>> 4. I wonder if javax.management.loading.MLet.loadLibraryAsResource and
>>> sun.net.www.MimeLauncher.run could be refactored with more appropriate
>>>
>>>      Files.copy(is, file.toPath())
>>>
>>> as it seems to fit there perfectly.
>> I will try to do so and send in a new Patch for this...
> It's absolutely fine for now. You can leave it as it is. java.nio.Files' time
> will come.
I've put this on my list of things to do later then...  ;-)
>>> 6. I've run into several cases where code use explicit `flush()` for
>>> ByteArrayOutputStream, BufferedOutputStream. For the former one it's never
>>> needed. The latter one does it automatically on `close()`.
>>> Should we still call them?
>> There I'm not really sure, as a matter of fact, the close() is being done on the try block exit and that is
>> being done after putting the return value on the stack as I believe (but I could be wrong).
>>
>> For the reason of my uncertainty I left the flush() method call in...
> Which one is that you're not sure about?
>
> BTW, looks like we've missed one more snippet in sun.print.UnixPrintJob:
>
>      try {
>          while ((cread = br.read(buffer, 0, buffer.length)) >=0) {
>              bw.write(buffer, 0, cread);
>          }
>          br.close();
>          bw.flush();
>          bw.close();
>      } catch (IOException e) {
>          notifyEvent(PrintJobEvent.JOB_FAILED);
>          throw new PrintException (e);
>      }
Sould I make the change or will you do it?
>>> 7. org.jcp.xml.dsig.internal.dom.Utils.readBytesFromStream is a little bit
>>> awkward since it used 1024 as some sort of a cut-off.
>>> It might've had something to do with EOF detection, but I'm not sure.
>> The way I was coming to the current implementation was based on due the fact, that we almost would have one read
>> operation iteration more in case the last read amount would be less than 1024 but instead having a comparison more
>> in each loop.
> The same thing as with (2). I don't like the package name. It might be synced
> from the outside of the JDK.
Seem reasonable to me too.
>>> It seems ok to remove the FileNotFoundException branch. Or I'm missing something.
>> No, I thing you're right due to the fact that the FileNotFoundException extends IOException anyway. Or would a
>> multi catch block make more sense?
>>
>>     } catch (FileNotFoundException | IOException e) {
>>        notifyEvent(PrintJobEvent.JOB_FAILED);
>>        throw new PrintException(e.toString());
>>     }
> I think we'd better remove it. As we both noticed, FileNotFoundException is a
> subtype of IOException.
That's totally fine for me
>> When looking closer to that code part now, I ask myself if it's a good thing to have the causing stack being lost
>> there and would it not be better to pass the causing exception?
>>
>>     } catch (FileNotFoundException | IOException e) {
>>        notifyEvent(PrintJobEvent.JOB_FAILED);
>>        throw new PrintException(e);
>>     }
> It would, you're right. Unfortunately we don't know how this exception might be
> used. Maybe there was a reason for not including the cause or it might have been
> just done before the "cause" field in Throwable became available (before
> JDK1.4)
I'm pretty sure about this. Here I guess we could ask the responsible 
group. Besides that, we could compose
the PrintException in a way, that the message stays the same and we 
additionally have the causing exception:

    } catch (IOException e) {
       notifyEvent(PrintJobEvent.JOB_FAILED);
       throw new PrintException(e.toString(), e);
    }


>>> 9. I don't think I would agree with some changes in sun.net.www.MimeLauncher.run
>>> as they obviously make the new version not equivalent to the old one. Non
>>> swallowed IOException, additional null check. Way too much for the cleanup
>>> change.
>> Well, the null check I agree. To have the same behaviour, we simply skip remove the null check.
>>
>> Your objection about the not swallowed IOException I can not see because it's being  swallowed in the
>> outer try-catch block at last. So I think that will result in the same result here.
> Yes, it's swallowed by the outer block. But as far as I can see it now doesn't
> do all these things after an exception is thrown from inside `transferTo`:
>
>      int inx = 0;
>      String c = execPath;
>      while ((inx = c.indexOf("%t")) >= 0) {
>          c = c.substring(0, inx) + uc.getContentType()
>              + c.substring(inx + 2);
>      }
>      
>      boolean substituted = false;
>      while ((inx = c.indexOf("%s")) >= 0) {
>          c = c.substring(0, inx) + ofn + c.substring(inx + 2);
>          substituted = true;
>      }
>      if (!substituted)
>          c = c + " <" + ofn;
>      
>      // System.out.println("Execing " +c);
>      
>      Runtime.getRuntime().exec(c);
Now I got the your point. I should stop writing emails and looking into 
code at 1 am ;-)

> -------------------------------------------------------------------------------
> P.S. And yes, Remi is right, we'd better split this thing into several pieces --
> according to areas.
I agree, the Linux find command does not stop on responsible groups 
boundaries ;-)

> Following 3-4 weeks I might become unresponsive on core-libs or privately as I'm
> busy with another project (just in case).
No problem, thanks for informing me. I also applied for author role, so 
that it's becoming easier, to maintain webrev's
by myself instead of posting diff's and you have to create again a 
webrev in turn....

> Thanks a lot for doing this, Patrick. It's really useful.
You're welcome, I'de love to some more in the future...

-Patrick