RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet

[Chris Hegarty]

> On 27 Sep 2016, at 19:56, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Chris,
> 
> thanks for your input.
> 
> If there's no objections I'd push it like this later tomorrow:
> http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/

Looks ok to me Christoph.

Thanks,
-Chris.

> I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full exception class names.
> 
> Best regards
> Christoph
> 
>> -----Original Message-----
>> From: Chris Hegarty [mailto:chris.hegarty at oracle.com]
>> Sent: Dienstag, 27. September 2016 10:10
>> To: Langer, Christoph <christoph.langer at sap.com>
>> Cc: net-dev at openjdk.java.net
>> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function
>> NET_ThrowSocketException in windows libnet
>> 
>> Christoph,
>> 
>> On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.langer at 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.hegarty at oracle.com]
>>>> Sent: Montag, 26. September 2016 16:51
>>>> To: Langer, Christoph <christoph.langer at sap.com>; net-
>> dev at 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.
>