<Sound Dev> Comment on bug JDK-8013586

[Florian Bomers]

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;

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
>> <klausj at phonetik.uni-muenchen.de
>> <mailto:klausj at 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 <danrollo at gmail.com
>>>> <mailto:danrollo at 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.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at 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: klausj at phonetik.uni-muenchen.de
> 

-- 
Florian Bomers
Bome Software

everything sounds.
http://www.bome.com
__________________________________________________________________
Bome Software GmbH & Co KG        Gesellschafterin:
Dachauer Str.187                  Bome Komplementär GmbH
80637 München, Germany            Geschäftsführung: Florian Bömers
Amtsgericht München HRA95502      Amtsgericht München HRB185574