Updating existing JDK code to use InputStream.transferTo() (original) (raw)

Patrick Reinhart patrick at reini.net
Thu May 14 10:05:08 UTC 2015

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, TRANSFERBUFFERSIZE)) >= 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.JOBFAILED); 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.JOBFAILED); 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.JOBFAILED); 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

More information about the core-libs-dev mailing list