Christoph, On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> wrote: > > Hi Chris, > > I agree with your comment on the NPE. It would probably be wrong. So I > restored the old code and also removed the comments suggesting the NPE. Here > is my new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/
This looks fine. > What I was thinking a bit more about after I posted my initial webrev was the > fact that the old NET_ThrowSocketException would register a GlobalRef to > java/net/SocketException whereas the other, more generic code would always > use the lookup by name. Would you think it is a performance benefit to keep a > reference to a standard exception class in some place and use it for throwing > or is it better to always look up the class? Throwing those kind of > exceptions is probably not on the hot path anyway - but on the other hand it > should be no issue to keep references to these very basic class types. What's > your view on that? I don’t believe that using a GlobalRef is worth it here. It adds a little complication, while not offering much benefit. JNU_ThrowByName should be fine. > And another probably aesthetic thing: I notice that sometimes a > JNU_JAVANETPKG "SocketException" is used and sometimes a > "java/net/SocketException", even within the same file like > SocketInputStream.c. Maybe I should unify this in the files that I touch here > and if yes, shall I use the literal name or the JNU_JAVANETPKG define? Any > opinion on that? My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. -Chris > Thanks for taking care of this, > Christoph > > >> -----Original Message----- >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >> Sent: Montag, 26. September 2016 16:51 >> To: Langer, Christoph <christoph.lan...@sap.com>; net-dev@openjdk.java.net >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >> NET_ThrowSocketException in windows libnet >> >> Christoph, >> >> On 22/09/16 21:59, Langer, Christoph wrote: >>> Hi, >>> >>> while looking at utility functions for creating exceptions in >>> libjava/libnet I found a small spot that should be consolidated right away. >>> >>> >>> The function NET_ThrowSocketException does only exist in the windows >>> native implementation and is only used in 3 places in >>> SocketInputStream.c. I removed this in favor of directly calling >>> JNU_ThrowByName as the Unix variant of that code already does. >>> >>> >>> In that function Java_java_net_SocketInputStream_socketRead0 I also >>> replaced throwing a SocketException with throwing an NPE in the rare >>> case that a the JNI input for the file descriptor is null. That's >>> probably more natural and should virtually never occur anyways. >> >> Hmmm... I'm not sure about this. SocketException is thrown on >> unix too for a similar situation. More significantly, a null value >> represents that the socket has been, possibly asynchronously, >> closed. >> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 >>> >>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ >> >> Other than the above concern, the remainder of the code looks ok >> to me. >> >> -Chris.
