<Sound Dev> [10] Review Request: 8191384 WaveFloatFileReader never closes the data stream

[Alex Menkov]

+1

--alex

On 11/19/2017 16:27, Dan Rollo wrote:
> Hi Sergey,
> 
> +1
> 
> Dan
> 
>> On Nov 19, 2017, at 2:58 AM, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
>>
>> Hi, Thank you for review.
>> Yes, you are right, I used my standard template to cover over most possible configurations and missed the case when the frameLength is not specified, the fix is updated:
>> http://cr.openjdk.java.net/~serb/8191384/webrev.01
>>
>> On 17/11/2017 13:27, Dan Rollo wrote:
>>> Hi Sergey,
>>> Overall looks good to me.
>>> I’m a little confused by the main() test loop:
>>> 115                         test(afw, afr, type, getStream(from, true));
>>> Did you also intended to call (frameLength = NOT_SPECIFIED), to would be:
>>> test(afw, afr, type, getStream(from, true));
>>> test(afw, afr, type, getStream(from, false));
>>> Otherwise, I don’t see where getStream(…, false) is used.
>>> -Dan
>>>> On Nov 17, 2017, at 3:11 AM, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
>>>>
>>>> Hello, Audio Guru.
>>>>
>>>> Please review the fix for jdk10.
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191384
>>>> Webrev can be found at: http://cr.openjdk.java.net/~serb/8191384/webrev
>>>>
>>>>
>>>> The bug occurs in the next code in WaveFloatFileReader.java:
>>>>     final RIFFReader riffiterator = new RIFFReader(stream);
>>>>     while (riffiterator.hasNextChunk()) {
>>>>        RIFFReader chunk = riffiterator.nextChunk();
>>>>        if (chunk.getFormat().equals("data")) {
>>>>            return new AudioInputStream(chunk, af, length);
>>>>        }
>>>>     }
>>>> In the code below we create a RIFFReader for the file stream, then iterates over the chunks to find a data chunk and create an AudioInputStream.
>>>>
>>>> The problem is that the chunk has noop close() method, so if the user will try to close AudioInputStream, the close() method of the file stream will not be called.
>>>>
>>>> In the fix the close() method for the chunk was changed and now it always close the stream on top of which it was created.
>>>> I also checked that this RIFFReader.close() method was never called in our code(except the tests), its probably a reason why the NPE when it is called twice was not found.
>>>>
>>>> -- 
>>>> Best regards, Sergey.
>>
>>
>> -- 
>> Best regards, Sergey.
>