8370655: Check EINTR handling InetAddress implementation and NET_ThrowNew #28750
Conversation
|
👋 Welcome back michaelm! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Michael-Mc-Mahon The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| icmp->icmp_cksum = in_cksum((u_short *)icmp, plen); | ||
| // send it | ||
| n = sendto(fd, sendbuf, plen, 0, &sa->sa, sizeof(struct sockaddr_in)); | ||
| while (1) { |
There was a problem hiding this comment.
Are you sure it is possible for sendto to block indefinitely here? Maybe EINTR is possible but I think it would be okay to retry unconditionally, meaning I don't think timerMillisExpired is needed here.
Are you sure that we restart for the other blocking syscalls (poll, recvfrom, ...) in these methods?
There was a problem hiding this comment.
Oops. Didn't notice I had left the other calls out. Will add them now.
As for sendto() it's a lot less likely to block and I doubt it can block indefinitely, but I imagine it can block if socket buffer is full. I don't have a strong preference. If EINTR can be returned maybe it should be handled the same way?
There was a problem hiding this comment.
As for sendto() it's a lot less likely to block and I doubt it can block indefinitely, but I imagine it can block if socket buffer is full. I don't have a strong preference. If EINTR can be returned maybe it should be handled the same way?
Hard to test but I suspect the packet will be dropped rather than blocking. So my guess is that retry here can be simple and doesn't need to use the timeout.
There was a problem hiding this comment.
getaddrinfo() and getnameinfo() can potentially return EINTR. The main blocking call in these files is NET_Wait, which is already restarting in the case of EINTR (along with all other errno values). The question is should we be returning an error if errno is not EINTR?
|
I’ve updated the implementation to check for EINTR from all blocking calls and all other system calls specified to be able to return EINTR, though in all cases they are used in non-blocking mode, which means the error is unlikely to occur. The new native function is removed and the test that exercised it also. There is some reformatting of NET_Wait but no functional changes. |
|
|
||
| error = getaddrinfo(hostname, NULL, &hints, &res); | ||
| NET_RESTARTABLE(error, getaddrinfo(hostname, NULL, &hints, &res), | ||
| error != EAI_SYSTEM) |
There was a problem hiding this comment.
Have you verified that a pthread_kill of a thread blocked in getaddrinfo returns EAI_SYSTEM with errno=EINTR? I can't be sure from the man page.
There was a problem hiding this comment.
Have you verified that a pthread_kill of a thread blocked in getaddrinfo returns EAI_SYSTEM with errno=EINTR? I can't be sure from the man page.
I will try to verify that.
2 participants
Hi,
This change updates the use of NET_ThrowNew in Inet4AddressImpl.c + Inet6AddressImpl.c (unix).
Currently EINTR is incorrectly handled in NET_ThrowNew to throw InterruptedIOException.
The only possible places in these files where EINTR can be returned is in the sendto() calls
for ping4() and ping6() used by the InetAddress.isReachable() API.
The change checks for EINTR returned from those calls and restarts the sendto()
if the timeout allows it. If EINTR is detected by NET_ThrowNew it is thrown as an ordinary
SocketException, but this should not happen.
The fix is only partially tested as it is difficult to make sendto() return EINTR in practice, but
I added a unit test for the new native function that checks if the timeout has expired.
Thanks,
Michael
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28750/head:pull/28750$ git checkout pull/28750Update a local copy of the PR:
$ git checkout pull/28750$ git pull https://git.openjdk.org/jdk.git pull/28750/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28750View PR using the GUI difftool:
$ git pr show -t 28750Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28750.diff
Using Webrev
Link to Webrev Comment