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.
Thanks,
Florian
On 16.07.2015 21:26, Sergey Bylokhov
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
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
<kla...@phonetik.uni-muenchen.de
<mailto: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,
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
--
Florian Bomers
Bome Software
everything sounds.
http://www.bome.com
__________________________________________________________________
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
|
