Thank you for the review, Anthony, I created new issue: https://bugs.openjdk.java.net/browse/JDK-8027469 and linked it together with 8027452.
Thanks, Oleg On 29.10.2013 21:00, Anthony Petrov wrote:
Thanks for the details, Oleg. I agree that it's a good idea to investigate the close() issue in JDK 9. I've also noticed that DataFlavor.getReaderForText() doesn't mention that user code is responsible for close()'ing the returned Reader. I suggest to update the specification of this method and mention that (please add a note about the spec change to 8027452 which you've just filed). With this done, the fix looks good to me. -- best regards, Anthony On 10/29/2013 06:39 PM, Oleg Pekhovskiy wrote:Hi Anthony, I could refer to the fix for JDK-7075105. the finally {} block was added there and had never existed before. Here's the way InputStream passed to DataTransferer.getInstance(). translateStream() gets closed: In out case DataTransferer.translateStream() calls DataTransferer.translateStreamToInputStream() on line 1776 where the stream is passed to ReencodingInputStream constructor. It is then passed to InputStreamReader constructor. Both classes override close() method where ReencodingInputStream closes referenced InputStreamReader and InputStreamReader closes referenced InputStream. ReencodingInputStream is returned by SunDropTargetContextPeer.getTransferData() method, called by DataFlavor.getReaderForText() that returns it to the client code. So it's a client code obligation to call ReencodingInputStream.close(). This is the use case for the test mentioned in the issue description. But in general we should investigate each use case to avoid memory leaks. As Petr pointed out it makes sense to do it for JDK 9, because it could be risky for now. Thanks, Oleg On 29.10.2013 16:41, Anthony Petrov wrote:Hi Oleg, I'm not an expert in this code so I may ask some silly questions. I'm OK with the change #2. Regarding #1: could you please clarify what code is responsible for closing the stream now, after your fix? Is this documented/enforced anywhere (i.e. a finally{} block or something)? Have you run any regression tests to make sure this change doesn't introduce any memory leaks? Why was this not a problem before that we decided to fix this particular piece now, so late in the release? -- best regards, Anthony On 10/28/2013 02:41 AM, Oleg Pekhovskiy wrote:Hi all, please review the fix http://cr.openjdk.java.net/~bagiras/8027151.1/ for https://bugs.openjdk.java.net/browse/JDK-8027151 This issue appeared after the changes made for JDK-7075105. There were two problems: 1. In drop target code, in SunDropTargetContextPeer.getTransferData() method inputStream was closed in finally scope but was not yet used in client code (indirectly). Just its reference stored for the future in DataTransferer.getInstance().translateStream() call. 2. In drop target code, in DataTransferer.translateStream() there was no String object handler (it was moved from DataTransferer.translateBytesOrStream() only to DataTransferer.translateBytes() method). Thanks, Oleg
