<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi, looks good to me, but I have only glanced over. This kind of
refactoring is error prone. It will be very good to have a series of
unit tests for that.<br>
Thanks,<br>
Florian<br>
<br>
<br>
<div class="moz-cite-prefix">On 16.07.2015 21:26, Sergey Bylokhov
wrote:<br>
</div>
<blockquote cite="mid:55A8055F.8040605@oracle.com" type="cite">Hi,
Everybody.
<br>
On 10.01.15 1:09, Florian Bomers wrote:
<br>
<blockquote type="cite">At the time, I've fixed the same type of
bug in WaveFileReader and the
<br>
likes. It was tracked under bug #4325421 and I *should* have
written a
<br>
unit test. If indeed, you should find it by looking for a unit
test
<br>
with that bug id.
<br>
<br>
In WaveFileReader, I fixed it without a clumsy catch clause --
<br>
analogous to this:
<br>
<br>
FileInputStream fis = new FileInputStream(file);
<br>
BufferedInputStream bis = new BufferedInputStream(fis);
<br>
AudioInputStream ais = null;
<br>
try {
<br>
ais = getAudioInputStream(bis);
<br>
} finally {
<br>
if (ais == null) {
<br>
bis.close();
<br>
}
<br>
}
<br>
return ais;
<br>
</blockquote>
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.
<br>
I have a small prototype where I merge all implementations of
getAudioXXX to the SunFileReader. It will fix this and related
issues:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~serb/8013586/webrev.01">http://cr.openjdk.java.net/~serb/8013586/webrev.01</a>
<br>
Issues which were fixed:
<br>
- Streams were not closed when necessary. In the fix they are
closed always in case of any exceptions. JDK-8013586
<br>
- Some of the readers do not reset the streams when they throw an
UnsupportedAudioFileException exception. JDK-8130305
<br>
- Some of the readers(like AiffFileReader) do not wrap
(FileInputStream, etc) in BufferedInputStream.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
Best, Florian On 09.01.2015 20:21, Dan Rollo wrote:
<br>
<blockquote type="cite">
<blockquote type="cite">Yikes, Good point Klaus! Forgot the
caller wants to actually use a
<br>
valid stream for the non-exceptional case. Would have to move
the
<br>
is.close() back into a catch clause. I’ll try to post a better
one
<br>
later. (Any unit tests of this sort of thing exist in the tree
now? -
<br>
if not, I could try a unit test too).
<br>
<br>
<blockquote type="cite">On Jan 9, 2015, at 12:34 PM, Klaus
Jaensch
<br>
<<a class="moz-txt-link-abbreviated" href="mailto:klausj@phonetik.uni-muenchen.de">klausj@phonetik.uni-muenchen.de</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:klausj@phonetik.uni-muenchen.de"><mailto:klausj@phonetik.uni-muenchen.de></a>> wrote:
<br>
<br>
Hi Dan,
<br>
<br>
Am 09.01.2015 um 05:37 schrieb Dan Rollo:
<br>
<blockquote type="cite">Even better, no need for the
“catch/throw” chunk, because the
<br>
method declares those caught exceptions:
<br>
<br>
<br>
public AudioInputStream getAudioInputStream(File file)
<br>
throws UnsupportedAudioFileException, IOException
{
<br>
<br>
final FileInputStream fis = new
FileInputStream(file);
<br>
try {
<br>
return getAudioInputStream(new
BufferedInputStream(fis));
<br>
} finally {
<br>
fis.close();
<br>
}
<br>
}
<br>
</blockquote>
I think your code will not work. If the call to
<br>
getAudioInputStream(InputStream) succeeds the code always
closes the
<br>
stream before it is returned.
<br>
<br>
<br>
I suggest this approach:
<br>
<br>
public AudioInputStream getAudioInputStream(File file)
<br>
throws UnsupportedAudioFileException,
IOException {
<br>
FileInputStream fis=new FileInputStream(file);
<br>
BufferedInputStream bis=new
BufferedInputStream(fis);
<br>
AudioInputStream ais=null;
<br>
try{
<br>
ais=getAudioInputStream(bis);
<br>
} catch(IOException|UnsupportedAudioFileException
e) {
<br>
if(bis!=null){
<br>
bis.close();
<br>
}
<br>
throw e;
<br>
}
<br>
return ais;
<br>
}
<br>
<br>
Closes the BufferedInputStream as well as the underlying
<br>
FileInputStream.
<br>
<br>
I think the method:
<br>
public AudioInputStream getAudioInputStream(URL url)
<br>
<br>
needs to be changed in the same way.
<br>
<br>
<br>
Best regards,
<br>
Klaus
<br>
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">On Jan 7, 2015, at 11:41 PM, Dan
Rollo <<a class="moz-txt-link-abbreviated" href="mailto:danrollo@gmail.com">danrollo@gmail.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:danrollo@gmail.com"><mailto:danrollo@gmail.com></a>> wrote:
<br>
<br>
If this approach is taken, I’d like to suggest using a
‘final’ var
<br>
instead of ‘null init/null check’, for example:
<br>
<br>
public AudioInputStream getAudioInputStream(File file)
<br>
throws UnsupportedAudioFileException,
IOException {
<br>
<br>
final FileInputStream fis = new
FileInputStream(file);
<br>
try {
<br>
return getAudioInputStream(new
BufferedInputStream(fis));
<br>
} catch(IOException|UnsupportedAudioFileException
e) {
<br>
throw e;
<br>
} finally {
<br>
fis.close();
<br>
}
<br>
}
<br>
<br>
<br>
<blockquote type="cite">On Jan 7, 2015, at 10:51 PM,
Sergey Bylokhov
<br>
<<a class="moz-txt-link-abbreviated" href="mailto:Sergey.Bylokhov@oracle.com">Sergey.Bylokhov@oracle.com</a>
<a class="moz-txt-link-rfc2396E" href="mailto:Sergey.Bylokhov@oracle.com"><mailto:Sergey.Bylokhov@oracle.com></a>>
<br>
wrote:
<br>
<br>
On 08.01.2015 1:13, Phil Race wrote:
<br>
<blockquote type="cite">Its not clear to me if the bug
description is implying an
<br>
exception was thrown
<br>
</blockquote>
UnsupportedAudioFileException is thrown if the
URL/File does not
<br>
point to valid audio file data recognized by the
specific reader,
<br>
so AudioSystem will try to move to the next reader and
a leak
<br>
will occur.
<br>
Actually most of our readers are affected.
<br>
<blockquote type="cite">Still, something like what you
suggest seems to be needed.
<br>
</blockquote>
right.
<br>
<blockquote type="cite">The owner of this bug is out
until next week so I'll let him
<br>
comment further
<br>
after his return.
<br>
<br>
-phil.
<br>
<br>
On 01/07/2015 12:42 PM, Mike Clark wrote:
<br>
<blockquote type="cite">Hello all,
<br>
<br>
I wanted to post this as a comment
<br>
on
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8013586">https://bugs.openjdk.java.net/browse/JDK-8013586</a>,
but
<br>
apparently getting comment access to that system
is a bit of a
<br>
hurdle. Anyway. What follows is, I believe, a
fix for the
<br>
aforementioned bug:
<br>
<br>
There is a file handle leak in some of the
subclasses of
<br>
javax.sound.sampled.spi.AudioFileReader, such as
<br>
com.sun.media.sound.WaveFloatFileReader.
<br>
<br>
Consider com.sun.media.sound.WaveFloatFileReader's
method:
<br>
<br>
public AudioInputStream getAudioInputStream(File
file)
<br>
throws UnsupportedAudioFileException,
IOException {
<br>
return getAudioInputStream(
<br>
new BufferedInputStream(new
FileInputStream(file)));
<br>
}
<br>
<br>
See how there is no attempt to close the
FileInputStream if an
<br>
exception is thrown? A file handle will remain
open on the
<br>
file until garbage collection is run. Since
garbage collection
<br>
may never run, the file handle may remain open
until the JVM
<br>
exits. And on Windows the open file handle
prevents the file
<br>
from being deleted, which is problematic.
<br>
<br>
Could we fix it by adding a try/catch block?
<br>
<br>
public AudioInputStream getAudioInputStream(File
file)
<br>
throws UnsupportedAudioFileException,
IOException {
<br>
FileInputStream fis = null;
<br>
try {
<br>
fis = new FileInputStream(file);
<br>
return getAudioInputStream(new
BufferedInputStream(fis));
<br>
}
catch(IOException|UnsupportedAudioFileException e)
{
<br>
if (fis != null) {
<br>
fis.close();
<br>
}
<br>
throw e;
<br>
}
<br>
}
<br>
<br>
These AudioFileReader subclass methods are usually
called by
<br>
javax.sound.sampled.AudioSystem.getAudioInputStream(File),
<br>
which calls getAudioInputStream(File) on all
registered
<br>
subclasses of AudioFileReader. As such, all
subclasses of
<br>
AudioFileReader in the JRE should be reviewed for
this problem.
<br>
<br>
best regards,
<br>
-Mike
<br>
<br>
</blockquote>
</blockquote>
<br>
-- <br>
Best regards, Sergey.
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
-- <br>
------------------------------------------
<br>
Klaus Jaensch
<br>
Muenchen
<br>
Germany
<br>
<br>
Institut fuer Phonetik und Sprachverarbeitung
<br>
Schellingstr.3/II
<br>
Room 223 VG
<br>
80799 München
<br>
<br>
Phone (Work): +49-(0)89-2180-2806
<br>
Fax: +49-(0)89-2180-5790
<br>
EMail: <a class="moz-txt-link-abbreviated" href="mailto:klausj@phonetik.uni-muenchen.de">klausj@phonetik.uni-muenchen.de</a>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
<br>
<pre class="moz-signature" cols="70">--
Florian Bomers
Bome Software
everything sounds.
<a class="moz-txt-link-freetext" href="http://www.bome.com">http://www.bome.com</a>
__________________________________________________________________
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
</pre>
</body>
</html>