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>
> 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
>>>>>> <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
> <mailto:kla...@phonetik.uni-muenchen.de>
