Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). I
have incorporated the review comments.
Thanks,
Vyom
On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:
On 05/09/16 15:37, Mark Sheppard wrote:
if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,
Yes, the desire is to make no more calls to gettimeofday, than are
already done.
then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
int result;
struct timeval t;
long prevtime, newtime;
struct pollfd pfd;
pfd.fd = s;
pfd.events = POLLIN;
if (timeout > 0) {
prevtime = (current_time->tv_sec * 1000) +
current_time->tv_usec / 1000;
}
for(;;) {
result = poll(&pfd, 1, timeout);
if (result < 0 && errno == EINTR) {
if (timeout > 0) {
gettimeofday(&t, NULL);
newtime = (t.tv_sec * 1000) + t.tv_usec /1000;
timeout -= newtime - prevtime;
if (timeout <= 0)
return 0;
prevtime = newtime;
}
} else {
return result;
}
}
}
the NET_ReadWithTimeout is modified with.
while (timeout > 0) {
result = NET_TimeoutWithCurrentTime(fd, timeout, &t);
in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,
Yes, and that is fine. Just no more than is already there.
and adjust the originally specified timeout
accordingly, and for the edge case
after the non blocking read.
After an initial skim, your suggestion seems reasonable.
-Chris.
regards
Mark
On 05/09/2016 12:02, Chris Hegarty wrote:
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