<Sound Dev> Comment on bug JDK-8013586
Dan Rollo
danrollo at gmail.com
Fri Jul 17 14:49:51 UTC 2015
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.Bylokhov at 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
>>>> <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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/sound-dev/attachments/20150717/f1e0625b/attachment-0001.html>
More information about the sound-dev
mailing list