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

[Chris Hegarty]

Vyom,

 >>> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

There is some concern about the potential performance effect, etc,
of gettimeofday, maybe there is a way out of this. The reuse of
NET_Timeout is good, but it also calls gettimeofday. It seems that
a specific NET_ReadWithTimeout could be written to NOT reuse
NET_Timeout, but effectively inline its interruptible operation.
Or write a variant of NET_Timeout that takes a function to
execute. Rather than effectively two loops conditioned on the
timeout.  Disclaimer: I have not actually tried to do this, but
it seems worth considering / evaluating.

-Chris.

On 02/09/16 04:39, Vyom Tewari wrote:
> hi Dimitry,
>
> thanks for review, I did consider to use  a monotonically increasing
> clock like "clock_gettime" but  existing nearby code("NET_Timeout") uses
> "gettimeofday"  so i choose to be consistent with the existing code.
>
> Thanks,
>
> Vyom
>
>
> On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:
>> Vyom,
>>
>> Did you consider to use select() to calculate timeout instead of
>> gettimeofday ?
>>
>> gettimeofday is affected by system time changes, so running ntpd can
>> cause unpredictable behavior of this code. Also it's rather expensive
>> syscall.
>>
>> -Dmitry
>>
>> On 2016-09-01 19: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
>>>>>>
>>>>>>
>>
>