Hi, looks good to me. The new unit test might be a little more verbose that it would throw an exception in the last Files.delete() statement. Still, as said, this kind of refactoring is error prone.
Always resetting the stream after reading the file format does not contradict the specification. The specification says that it may need to mark and reset the stream. It is up to the implementation if and when any of that is actually done. Because there is no functional difference and to not accidentally introduce a regression with applications relying on the current behavior, I'd keep it as it is. There is a certain consistency in it when getAudioFileFormat always resets the stream. WaveExtensibleFileReader -- I assume it got imported with gervill. Such Wave files are rather rare. Thanks, Florian On 21.07.2015 17:17, Sergey Bylokhov wrote: > Hello. > Please review the fix for jdk9. Two issues were fixed: > 8013586: audioInputStream.close() does not release the resource > 8130305: AudioSystem behavior depends on order that providers are located > > Description: > - Streams were not closed when necessary(ex: WaveFloatFileReader. > getAudioInputStream(File)). In the fix they are closed always in case > of any exceptions. > - Some of the readers do not reset the streams when they throw an > UnsupportedAudioFileException exception. > - Some of the readers(like AiffFileReader) do not wrap > (FileInputStream, etc) in BufferedInputStream. > > In the fix I merge all implementations of getAudioXXX to the > SunFileReader. > > Note that I added a comment to the > SunFileReader.getAudioFileFormat(InputStream) that implementation > contradicts the specification, and it seems the javadoc should be > updated? And I created a new issue about EOFException: > https://bugs.openjdk.java.net/browse/JDK-8131974 > > 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. Is that intentionally? > > Bug: https://bugs.openjdk.java.net/browse/JDK-8013586 > Webrev can be found at: http://cr.openjdk.java.net/~serb/8013586/webrev.02 > > -- > Best regards, Sergey. > -- 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
