<Sound Dev> Comment on bug JDK-8013586

[Klaus Jaensch]

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 
>>>>> onhttps://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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/sound-dev/attachments/20150109/874dd246/attachment-0001.html>