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

[Vyom Tewari]

Hi Mark,

Thanks for the review, even i don't like "goto" , i wanted to do code 
changes as less as possible(not even refactoring) and "goto" helped me 
in this.

I will incorporate the review comment and  send the modified webrev.

Thanks,
Vyom

On 8/30/2016 4: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/20160830/2ddaa7b7/attachment.html>