RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

On Friday 12 October 2018 04:31 PM, Chris Hegarty wrote:
> Daniel,
>
> On 12/10/18 11:26, Daniel Fuchs wrote:
>> Hi,
>>
>> Maybe a more conservative implementation could have been:
>>
>>     int available = impl.available();
>>     return eof ? 0 : available;
>
> That buys us little more than we had prior to this change,
> since impl.available will still call into native before
> checking the EOF status.
>
> If we want to keep this, then we need:
>
>     public int available() throws IOException {
>         if (impl.isClosedOrPending()) {
>             throw new SocketException("Socket closed");
>         }
>
>         if (eof) {
>             return 0;
>         } else {
>             return impl.available();
>         }
>     }
>
Above change is working as expected, i  modified the "CloseAvailable"  
test little bit and with modified code, SocketInputstream.available() is 
now throwing "java.net.SocketException: Socket closed" exception if the 
underline socket is closed.

I will prepare a patch along with modified test and send it for review.

I ran our internal tests and it looks like  it was not caught there.

Thanks,
Vyom

>> I almost suggested that yesterday, but I saw that
>> read() already had a logic similar to what Vyom was
>> proposing for available:
>>
>>   146         // EOF already encountered
>>   147         if (eof) {
>>   148             return -1;
>>   149         }
>>
>> which AFAICT  means that read returns -1 instead of throwing
>> if the socket is closed and EOF was previously reached.
>
> That is correct. While not intuitive, I don't propose
> that we change this. ( if this were a new implementation
> then I think it should throw IOE for this scenario, but
> we are where we are ).
>
> -Chris.
>
>
>> best regards,
>>
>> -- daniel
>>
>> On 12/10/2018 09:55, Chris Hegarty wrote:
>>>
>>> On 12/10/18 08:29, Alan Bateman wrote:
>>>> On 11/10/2018 09:03, vyom tewari wrote:
>>>>> Hi Chris,
>>>>>
>>>>> Thanks for review, please find the updated 
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8189366/webrev0.1/index.html) 
>>>>> where i included the test.
>>>> Can you explain the behavior change for the closed socket case? 
>>>> Will this change mean that available returns 0 when it previously 
>>>> throw IOException?
>>>
>>> You are correct. This is not intentional, or desirable.
>>>
>>> We should revert the change or add an additional check
>>> for isClosedOrPending. Since this is already pushed, I
>>> filed a new JIRA issue to track this.
>>>
>>>    https://bugs.openjdk.java.net/browse/JDK-8212114
>>>
>>> -Chris.
>>>
>>