Re: 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