<Sound Dev> Comment on bug JDK-8013586

[Sergey Bylokhov]

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
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
>>> <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


-- 
Best regards, Sergey.