-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8317603: Improve exception messages thrown by sun.nio.ch.Net native methods (win) #16057
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, the "no further information" message wasn't pretty.
I'm not a fan of mixing styles, and this file uses NET_ThrowNew
a lot. Can we choose either NET_ThrowNew
or handleSocketErrorMessage
and use it consistently everywhere?
Also, the NET_ThrowNew
messages usually list the function name only, without the failed
suffix. Can we strip that suffix here?
Hi Daniel, thanks for looking into it. I switched my calls to NET_ThrowNew and removed the 'failed' suffix of the message. |
@@ -727,7 +729,7 @@ Java_sun_nio_ch_Net_pollConnect(JNIEnv* env, jclass this, jobject fdo, jlong tim | |||
NET_ThrowNew(env, lastError, "getsockopt"); | |||
} | |||
} else if (optError != NO_ERROR) { | |||
handleSocketError(env, optError); | |||
NET_ThrowNew(env, optError, "getsockopt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a getsockopt
error, and using "getsockopt"
here will make errors thrown here indistinguishable from errors thrown a few lines above. Can we use a different text?
The errors here will usually be the result of async connect, but I'm not sure if that's guaranteed; SO_ERROR
would probably be the safest choice, but it doesn't look pretty. I guess that's one of the places where "no further information" actually makes sense.
Other than this, LGTM.
@@ -584,7 +586,7 @@ Java_sun_nio_ch_Net_getInterface4(JNIEnv* env, jobject this, jobject fdo) | |||
|
|||
n = getsockopt(fdval(env, fdo), IPPROTO_IP, IP_MULTICAST_IF, (void*)&in, &arglen); | |||
if (n == SOCKET_ERROR) { | |||
handleSocketError(env, WSAGetLastError()); | |||
NET_ThrowNew(env, WSAGetLastError(), "setsockopt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is typo, third argument should be "getsockopt"
Thanks, I adjusted the 2 messages mentioned . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me.
Thanks for the review ! |
I plan to review this as I think we'll need to do a few other changes as part of this. |
Moving to using NET_ThrowNew is okay but it creates inconsistencies that have to be fixed. I think this PR will go through a few iterations. Some background here is that the socket code no longer exists in libnet/net.dll and at some point we need to do some cleanup and refactoring so that libnio does not have libnet as a dependency (if you go through the JBS issue you'll see a number of startup/first-use issues caused by this dependency). We don't have to do this as part of this PR of course but it provides some context. For this PR, the issue with changing the specific functions to use NET_ThrowNet is that handleSocketError is left in place for DatagramChannelImpl.c, IOUtil.c and UnixDomainSocket.c. We shouldn't do that. We either replace usages (and remove handleSocketError) or change the existing function to take an additional message. |
@@ -633,7 +635,7 @@ Java_sun_nio_ch_Net_available(JNIEnv *env, jclass cl, jobject fdo) | |||
{ | |||
int count = 0; | |||
if (NET_SocketAvailable(fdval(env, fdo), &count) != 0) { | |||
handleSocketError(env, WSAGetLastError()); | |||
NET_ThrowNew(env, WSAGetLastError(), "socket availability check"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "ioctlsocket", not ""socket availability check". Better still would be to just replace the usage of NET_SocketAvailable to use ioctlsocket directly as we will eventually remove NET_SocketAvailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay then let's use ioctlsocket . We could completely remove NET_SocketAvailable. but in a follow up.
@@ -554,7 +556,7 @@ Java_sun_nio_ch_Net_blockOrUnblock6(JNIEnv *env, jobject this, jboolean block, j | |||
int opt = (block) ? MCAST_BLOCK_SOURCE : MCAST_UNBLOCK_SOURCE; | |||
int n = setGroupSourceReqOption(env, fdo, opt, group, index, source); | |||
if (n == SOCKET_ERROR) { | |||
handleSocketError(env, WSAGetLastError()); | |||
NET_ThrowNew(env, WSAGetLastError(), "setsockopt with group source request"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That message looks odd, I think you want "setsocketopt to block or unblock source".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I adjusted the message.
Okay, let's remove then the usages in the files you mentioned. |
@@ -89,7 +89,7 @@ Java_sun_nio_ch_DatagramChannelImpl_disconnect0(JNIEnv *env, jclass clazz, | |||
|
|||
rv = connect((SOCKET)fd, &sa.sa, sa_len); | |||
if (rv == SOCKET_ERROR) { | |||
handleSocketError(env, WSAGetLastError()); | |||
NET_ThrowNew(env, WSAGetLastError(), "connect"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check the copyright date on this and the other files as some of these haven't been updated in a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alan, I adjusted the copyright dates.
if (block) { | ||
NET_ThrowNew(env, WSAGetLastError(), "setsocketopt to block source"); | ||
} else { | ||
NET_ThrowNew(env, WSAGetLastError(), "setsocketopt to unblock source"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be reduced to one message (block or unblock) source as it doesn't add value as the stack trace will make it very clear if a source-specific block or unblock op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I reduced it to one message.
handleSocketError(env, WSAGetLastError()); | ||
u_long arg; | ||
if (ioctlsocket((SOCKET) fdval(env, fdo), FIONREAD, &arg) == SOCKET_ERROR) { | ||
NET_ThrowNew(env, WSAGetLastError(), "ioctlsocket"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, we should have removed the use of NET_SocketAvailable here a long time ago (same thing in the Unix code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think this version looks okay. We should probably rename the JBS issue and the PR as the change is no longer a handleSocketWithMessage function.
Hi reviewers , are you fine with the latest version ? |
@@ -727,7 +723,7 @@ Java_sun_nio_ch_Net_pollConnect(JNIEnv* env, jclass this, jobject fdo, jlong tim | |||
NET_ThrowNew(env, lastError, "getsockopt"); | |||
} | |||
} else if (optError != NO_ERROR) { | |||
handleSocketError(env, optError); | |||
NET_ThrowNew(env, optError, "getsockopt issue with option value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume "getsockopt issue with option value" should be "getsockopt" here.
} | ||
|
||
if (n == SOCKET_ERROR) { | ||
handleSocketError(env, WSAGetLastError()); | ||
NET_ThrowNew(env, WSAGetLastError(), error_message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I this this one can be simplified to "setsockopt". Source-specific multicast isn't widely used but if you did happen to get a failure here then the stack trace should make it very clear as it's a different join method.
Hi Alan, I simplified the 2 messages. |
@@ -539,10 +533,11 @@ Java_sun_nio_ch_Net_joinOrDrop6(JNIEnv *env, jobject this, jboolean join, jobjec | |||
} else { | |||
int opt = (join) ? MCAST_JOIN_SOURCE_GROUP : MCAST_LEAVE_SOURCE_GROUP; | |||
n = setGroupSourceReqOption(env, fdo, opt, group, index, source); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this blank line isn't needed now.
@@ -554,7 +549,7 @@ Java_sun_nio_ch_Net_blockOrUnblock6(JNIEnv *env, jobject this, jboolean block, j | |||
int opt = (block) ? MCAST_BLOCK_SOURCE : MCAST_UNBLOCK_SOURCE; | |||
int n = setGroupSourceReqOption(env, fdo, opt, group, index, source); | |||
if (n == SOCKET_ERROR) { | |||
handleSocketError(env, WSAGetLastError()); | |||
NET_ThrowNew(env, WSAGetLastError(), "setsocketopt to block or unblock source"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is okay although it can be reduced to "setsockopt" to be consistent with the others. If source filtering is used then the stack trace will be very clear if a source address is being blocked or unblocked.
@MBaesken This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 37 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thanks for the reviews! /integrate |
Going to push as commit a9b41da.
Your commit was automatically rebased without conflicts. |
created #16057 (review) for the suggested NET_SocketAvailable removal/replacement . |
On Windows, we miss a handleSocketErrorWithMessage function that provides an additional message showing what went wrong in the Net.c coding. On Unix we have this function.
This leads sometimes to exceptions like
MSG RTE: javax.naming.CommunicationException: example.com:1234 [Root exception is java.net.ConnectException: Connection timed out: no further information]
see https://bugs.openjdk.org/browse/JDK-8317307
It would be better to have a message explaining the reason instead of "no further information" .
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16057/head:pull/16057
$ git checkout pull/16057
Update a local copy of the PR:
$ git checkout pull/16057
$ git pull https://git.openjdk.org/jdk.git pull/16057/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16057
View PR using the GUI difftool:
$ git pr show -t 16057
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16057.diff
Webrev
Link to Webrev Comment