It’s great to see activity on this.
Any chance of adding some unit tests of the changes? (Could stream mocks help?)
This code and related “.close()” logic has be a challenge for a long time.
Might be worth tweaking to make it more testable now (even if that involved
something like a new package visible method that takes an easily mocked stream
object - instead of a file…).
Dan
> On Jul 16, 2015, at 3:26 PM, Sergey Bylokhov <sergey.bylok...@oracle.com>
> wrote:
>
> Hi, Everybody.
> On 10.01.15 1:09, Florian Bomers wrote:
>> At the time, I've fixed the same type of bug in WaveFileReader and the
>> likes. It was tracked under bug #4325421 and I *should* have written a
>> unit test. If indeed, you should find it by looking for a unit test
>> with that bug id.
>>
>> In WaveFileReader, I fixed it without a clumsy catch clause --
>> analogous to this:
>>
>> FileInputStream fis = new FileInputStream(file);
>> BufferedInputStream bis = new BufferedInputStream(fis);
>> AudioInputStream ais = null;
>> try {
>> ais = getAudioInputStream(bis);
>> } finally {
>> if (ais == null) {
>> bis.close();
>> }
>> }
>> return ais;
> In this example if getAudioInputStream() will throw
> UnsupportedAudioFileException and the last close() method will throw
> IOException, then we will not check the next audio reader.
> I have a small prototype where I merge all implementations of getAudioXXX to
> the SunFileReader. It will fix this and related issues:
> http://cr.openjdk.java.net/~serb/8013586/webrev.01
> <http://cr.openjdk.java.net/~serb/8013586/webrev.01>
> Issues which were fixed:
> - Streams were not closed when necessary. In the fix they are closed always
> in case of any exceptions. JDK-8013586
> - Some of the readers do not reset the streams when they throw an
> UnsupportedAudioFileException exception. JDK-8130305
> - Some of the readers(like AiffFileReader) do not wrap (FileInputStream, etc)
> in BufferedInputStream.
>
> I assume that this fix should not cause regressions so I will send a review
> request for it as is, after the testing. I found some other small issues(like
> handling EOFException) in this code but will fix it separately later.
>
> Also I have a related question about WaveExtensibleFileReader why it was not
> added to the spi.AudioFileReader? Because of this WaveExtensibleFileReader
> actually is never used.
>
> Best, Florian On 09.01.2015 20:21, Dan Rollo wrote:
>>> Yikes, Good point Klaus! Forgot the caller wants to actually use a
>>> valid stream for the non-exceptional case. Would have to move the
>>> is.close() back into a catch clause. I’ll try to post a better one
>>> later. (Any unit tests of this sort of thing exist in the tree now? -
>>> if not, I could try a unit test too).
>>>
>>>> On Jan 9, 2015, at 12:34 PM, Klaus Jaensch
>>>> <kla...@phonetik.uni-muenchen.de
>>>> <mailto:kla...@phonetik.uni-muenchen.de>> wrote:
>>>>
>>>> Hi Dan,
>>>>
>>>> Am 09.01.2015 um 05:37 schrieb Dan Rollo:
>>>>> Even better, no need for the “catch/throw” chunk, because the
>>>>> method declares those caught exceptions:
>>>>>
>>>>>
>>>>> public AudioInputStream getAudioInputStream(File file)
>>>>> throws UnsupportedAudioFileException, IOException {
>>>>>
>>>>> final FileInputStream fis = new FileInputStream(file);
>>>>> try {
>>>>> return getAudioInputStream(new BufferedInputStream(fis));
>>>>> } finally {
>>>>> fis.close();
>>>>> }
>>>>> }
>>>> I think your code will not work. If the call to
>>>> getAudioInputStream(InputStream) succeeds the code always closes the
>>>> stream before it is returned.
>>>>
>>>>
>>>> I suggest this approach:
>>>>
>>>> public AudioInputStream getAudioInputStream(File file)
>>>> throws UnsupportedAudioFileException, IOException {
>>>> FileInputStream fis=new FileInputStream(file);
>>>> BufferedInputStream bis=new BufferedInputStream(fis);
>>>> AudioInputStream ais=null;
>>>> try{
>>>> ais=getAudioInputStream(bis);
>>>> } catch(IOException|UnsupportedAudioFileException e) {
>>>> if(bis!=null){
>>>> bis.close();
>>>> }
>>>> throw e;
>>>> }
>>>> return ais;
>>>> }
>>>>
>>>> Closes the BufferedInputStream as well as the underlying
>>>> FileInputStream.
>>>>
>>>> I think the method:
>>>> public AudioInputStream getAudioInputStream(URL url)
>>>>
>>>> needs to be changed in the same way.
>>>>
>>>>
>>>> Best regards,
>>>> Klaus
>>>>
>>>>
>>>>>
>>>>>> On Jan 7, 2015, at 11:41 PM, Dan Rollo <danro...@gmail.com
>>>>>> <mailto:danro...@gmail.com>> wrote:
>>>>>>
>>>>>> If this approach is taken, I’d like to suggest using a ‘final’ var
>>>>>> instead of ‘null init/null check’, for example:
>>>>>>
>>>>>> public AudioInputStream getAudioInputStream(File file)
>>>>>> throws UnsupportedAudioFileException, IOException {
>>>>>>
>>>>>> final FileInputStream fis = new FileInputStream(file);
>>>>>> try {
>>>>>> return getAudioInputStream(new BufferedInputStream(fis));
>>>>>> } catch(IOException|UnsupportedAudioFileException e) {
>>>>>> throw e;
>>>>>> } finally {
>>>>>> fis.close();
>>>>>> }
>>>>>> }
>>>>>>
>>>>>>
>>>>>>> On Jan 7, 2015, at 10:51 PM, Sergey Bylokhov
>>>>>>> <sergey.bylok...@oracle.com <mailto:sergey.bylok...@oracle.com>>
>>>>>>> wrote:
>>>>>>>
>>>>>>> On 08.01.2015 1:13, Phil Race wrote:
>>>>>>>> Its not clear to me if the bug description is implying an
>>>>>>>> exception was thrown
>>>>>>> UnsupportedAudioFileException is thrown if the URL/File does not
>>>>>>> point to valid audio file data recognized by the specific reader,
>>>>>>> so AudioSystem will try to move to the next reader and a leak
>>>>>>> will occur.
>>>>>>> Actually most of our readers are affected.
>>>>>>>> Still, something like what you suggest seems to be needed.
>>>>>>> right.
>>>>>>>> The owner of this bug is out until next week so I'll let him
>>>>>>>> comment further
>>>>>>>> after his return.
>>>>>>>>
>>>>>>>> -phil.
>>>>>>>>
>>>>>>>> On 01/07/2015 12:42 PM, Mike Clark wrote:
>>>>>>>>> Hello all,
>>>>>>>>>
>>>>>>>>> I wanted to post this as a comment
>>>>>>>>> on https://bugs.openjdk.java.net/browse/JDK-8013586, but
>>>>>>>>> apparently getting comment access to that system is a bit of a
>>>>>>>>> hurdle. Anyway. What follows is, I believe, a fix for the
>>>>>>>>> aforementioned bug:
>>>>>>>>>
>>>>>>>>> There is a file handle leak in some of the subclasses of
>>>>>>>>> javax.sound.sampled.spi.AudioFileReader, such as
>>>>>>>>> com.sun.media.sound.WaveFloatFileReader.
>>>>>>>>>
>>>>>>>>> Consider com.sun.media.sound.WaveFloatFileReader's method:
>>>>>>>>>
>>>>>>>>> public AudioInputStream getAudioInputStream(File file)
>>>>>>>>> throws UnsupportedAudioFileException, IOException {
>>>>>>>>> return getAudioInputStream(
>>>>>>>>> new BufferedInputStream(new FileInputStream(file)));
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> See how there is no attempt to close the FileInputStream if an
>>>>>>>>> exception is thrown? A file handle will remain open on the
>>>>>>>>> file until garbage collection is run. Since garbage collection
>>>>>>>>> may never run, the file handle may remain open until the JVM
>>>>>>>>> exits. And on Windows the open file handle prevents the file
>>>>>>>>> from being deleted, which is problematic.
>>>>>>>>>
>>>>>>>>> Could we fix it by adding a try/catch block?
>>>>>>>>>
>>>>>>>>> public AudioInputStream getAudioInputStream(File file)
>>>>>>>>> throws UnsupportedAudioFileException, IOException {
>>>>>>>>> FileInputStream fis = null;
>>>>>>>>> try {
>>>>>>>>> fis = new FileInputStream(file);
>>>>>>>>> return getAudioInputStream(new BufferedInputStream(fis));
>>>>>>>>> } catch(IOException|UnsupportedAudioFileException e) {
>>>>>>>>> if (fis != null) {
>>>>>>>>> fis.close();
>>>>>>>>> }
>>>>>>>>> throw e;
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> These AudioFileReader subclass methods are usually called by
>>>>>>>>> javax.sound.sampled.AudioSystem.getAudioInputStream(File),
>>>>>>>>> which calls getAudioInputStream(File) on all registered
>>>>>>>>> subclasses of AudioFileReader. As such, all subclasses of
>>>>>>>>> AudioFileReader in the JRE should be reviewed for this problem.
>>>>>>>>>
>>>>>>>>> best regards,
>>>>>>>>> -Mike
>>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>
>>>> --
>>>> ------------------------------------------
>>>> Klaus Jaensch
>>>> Muenchen
>>>> Germany
>>>>
>>>> Institut fuer Phonetik und Sprachverarbeitung
>>>> Schellingstr.3/II
>>>> Room 223 VG
>>>> 80799 München
>>>>
>>>> Phone (Work): +49-(0)89-2180-2806
>>>> Fax: +49-(0)89-2180-5790
>>>> EMail: kla...@phonetik.uni-muenchen.de
>
>
> --
> Best regards, Sergey.
