RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Mark Sheppard]

Hi Vyom,
      thanks for doing the refactoring.
changes look OK to me

regards
Mark

On 01/09/2016 17:03, Vyom Tewari wrote:
>
> please find the updated 
> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html 
> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). 
> I incorporated the review comments.
>
> Thanks,
>
> Vyom
>
>
> On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:
>>
>> Hi
>>   perhaps there is an opportunity to do  some refactoring here (...  
>> for me a "goto " carries a code smell! )
>>
>> along the lines
>>
>> if (timeout) {
>>     nread =  NET_ReadWithTimeout(...);
>> } else {
>>      nread = NET_Read(...);
>> }
>>
>>
>> the NET_ReadWithTimeout (...) function will contain a restructuring 
>> of your goto loop
>> while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
>>            if (nread <= 0) {
>>                if (nread == 0) {
>>                    JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
>>                                "Read timed out");
>>                } else if (nread == -1) {
>>                    if (errno == EBADF) {
>>                         JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", "Socket closed");
>>                    } else if (errno == ENOMEM) {
>>                        JNU_ThrowOutOfMemoryError(env, "NET_Timeout native heap allocation failed");
>>                    } else {
>>                        JNU_ThrowByNameWithMessageAndLastError
>>                            (env, JNU_JAVANETPKG "SocketException", "select/poll failed");
>>                    }
>>                }
>>                  // release buffer in main call flow
>> //              if (bufP != BUF) {
>> //                  free(bufP);
>> //             }
>>               nread = -1;
>>               break;
>>            } else {
>> nread = NET_NonBlockingRead(fd, bufP, len);
>> if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
>>                gettimeofday(&t, NULL);
>> newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
>> _timeout -= newtime - prevtime;
>> if(_timeout > 0){
>> prevtime = newtime;
>>                    }
>> } else { break; } } } return nread;
>>
>> e&oe
>>
>> regards
>> Mark
>>
>>
>> On 29/08/2016 10:58, Vyom Tewari wrote:
>>> gentle reminder, please review the below code change.
>>>
>>> Vyom
>>>
>>>
>>> On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:
>>>> Hi All,
>>>>
>>>> Please review the code changes for below issue.
>>>>
>>>> Bug         : https://bugs.openjdk.java.net/browse/JDK-8075484
>>>>
>>>> webrev    : 
>>>> http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html 
>>>> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>
>>>>
>>>> This issue is SocketInputStream.socketread0() hangs even with 
>>>> "soTimeout" set.the implementation of 
>>>> Java_java_net_SocketInputStream_socketRead0 assumes that read() 
>>>> won't block after poll() reports that a read is possible.
>>>>
>>>> This assumption does not hold, as noted on the man page for select 
>>>> (referenced by the man page for poll): Under Linux, select() may 
>>>> report a socket file descriptor as "ready for reading", while 
>>>> nevertheless a subsequent read blocks. This could for example 
>>>> happen when data has arrived but upon examination has wrong 
>>>> checksum and is discarded.
>>>>
>>>> Thanks,
>>>>
>>>> Vyom
>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160901/46eb4ad2/attachment-0001.html>