RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c

[Ivan Gerasimov]

Hello!

The function Java_java_net_NetworkInterface_getByInetAddress0 may throw, 
so after calling it we need to check if an exception is pending.


Would you please help review a one-line fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/

Thanks in advance!

--
With kind regards,
Ivan Gerasimov

Implementing a custom SocketImpl

[Lundell, Jared]

I have a use case where I need to implement a custom SocketImpl class and I've 
run into some difficulty caused by the current design of SocketImpl and the 
related classes in OpenJDK.  My use case is implementing a custom proxy 
protocol, similar in spirit to what SocksSocketImpl does but using a different 
wire protocol.  I need to be able to swap this proxy in underneath some 
existing code that I can't modify, which is why I think a SocketImpl is the 
appropriate solution rather than a SocketFactory (which would be much easier if 
I could modify the code in question).
 
SocketImpl is a public class, so I think I'm fairly safe in assuming it's 
intended as an extension point for application authors, but in practice it 
seems very difficult to actually extend.  Since I'm trying to accomplish 
something very similar to what SocksSocketImpl does I'd like to take the same 
approach that it and HttpConnectSocketImpl use of delegating all of the OS 
interactions to the base PlainSocketImpl class and just adding the 
customizations I need on top.  However, I can't extend PlainSocketImpl the way 
those other classes do because it is package-private and I can't create new 
classes in java.net.  Encapsulating a PlainSocketImpl doesn't really seem 
viable either since all of the methods are also protected or package-private.  
The alternatives I can see are to either implement a SocketImpl completely from 
scratch (including all the native code) or to resort to classloader hacks to be 
able to add a class to java.net which extends PlainSocketImpl.  Neither of 
those are particularly appealing.
 
I was going to ask whether it would be reasonable to make PlainSocketImpl 
public, but reading through the net-dev archives[1] I discovered that there's 
some refactoring[2] of SocketImpl that's happening right now to prepare for 
Project Loom related work[3].  That change is introducing DelegatingSocketImpl 
and PlatformSocketImpl classes and changes SocksSocketImpl and 
HttpConnectSocketImpl to extend DelegatingSocketImpl rather than 
PlainSocketImpl.  I'd love to be able to extend DelegatingSocketImpl for my use 
case, but it's package-private too, nor does there appear to be a way to get a 
PlatformSocketImpl.
 
Is there any way that we could make it easier to implement a custom SocketImpl 
(or am I missing some other possible approach)?  If so, would it be reasonable 
to make DelegatingSocketImpl public and introduce a DefaultSocketImplFactory 
that can be used to construct a PlatformSocketImpl?
 
Thanks,
Jared
 
PS I just joined the net-dev mailing list, so there may be some historical 
context I've missed.
 
[1] https://mail.openjdk.java.net/pipermail/net-dev/2019-March/012218.html
[2] http://cr.openjdk.java.net/~alanb/8220493/0/webrev/
[3] http://openjdk.java.net/jeps/8218559

RE: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c

[Langer, Christoph]

Hi Ivan,

looks good to me.

Best regards
Christoph

> -Original Message-
> From: net-dev  On Behalf Of Ivan
> Gerasimov
> Sent: Mittwoch, 20. März 2019 17:18
> To: net-dev 
> Subject: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c
> 
> Hello!
> 
> The function Java_java_net_NetworkInterface_getByInetAddress0 may
> throw,
> so after calling it we need to check if an exception is pending.
> 
> Would you please help review a one-line fix?
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
> WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/
> 
> Thanks in advance!
> 
> --
> With kind regards,
> Ivan Gerasimov

Re: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c

[Vyom Tiwari]

Hi Ivan,

Looks OK to me,  in case of exception "ni"  will be null  you can use the
macro(CHECK_NULL_RETURN) if you are not clearing the JNI exception, this
will save at least one additional function( "(*env)->ExceptionOccurred(env)"
) call.

Thanks,
Vyom

On Wed, Mar 20, 2019 at 9:49 PM Ivan Gerasimov 
wrote:

> Hello!
>
> The function Java_java_net_NetworkInterface_getByInetAddress0 may throw,
> so after calling it we need to check if an exception is pending.
>
> Would you please help review a one-line fix?
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
> WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/
>
> Thanks in advance!
>
> --
> With kind regards,
> Ivan Gerasimov
>
>

-- 
Thanks,
Vyom

Re: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c

[Ivan Gerasimov]

Thanks Vyom!


On 3/20/19 10:13 PM, Vyom Tiwari wrote:

Hi Ivan,

Looks OK to me,  in case of exception "ni"  will be null you can use 
the macro(CHECK_NULL_RETURN) if you are not clearing the JNI 
exception, this will save at least one additional function( 
"(*env)->ExceptionOccurred(env)" ) call.


Java_java_net_NetworkInterface_getByInetAddress0 may return NULL if 
there were no interfaces found.

We should not return from getMulticastInterface() in this case.

With kind regards,
Ivan



Thanks,
Vyom

On Wed, Mar 20, 2019 at 9:49 PM Ivan Gerasimov 
mailto:ivan.gerasi...@oracle.com>> wrote:


Hello!

The function Java_java_net_NetworkInterface_getByInetAddress0 may
throw,
so after calling it we need to check if an exception is pending.

Would you please help review a one-line fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/


Thanks in advance!

-- 
With kind regards,

Ivan Gerasimov



--
Thanks,
Vyom


--
With kind regards,
Ivan Gerasimov

Re: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c

[Vyom Tiwari]

Hi Ivan,

thanks for explanation, code change looks good to me.

Thanks,
Vyom

On Thu, Mar 21, 2019 at 11:26 AM Ivan Gerasimov 
wrote:

> Thanks Vyom!
>
> On 3/20/19 10:13 PM, Vyom Tiwari wrote:
>
> Hi Ivan,
>
> Looks OK to me,  in case of exception "ni"  will be null  you can use the
> macro(CHECK_NULL_RETURN) if you are not clearing the JNI exception, this
> will save at least one additional function( "
> (*env)->ExceptionOccurred(env)" ) call.
>
>
> Java_java_net_NetworkInterface_getByInetAddress0 may return NULL if there
> were no interfaces found.
> We should not return from getMulticastInterface() in this case.
>
> With kind regards,
> Ivan
>
>
> Thanks,
> Vyom
>
> On Wed, Mar 20, 2019 at 9:49 PM Ivan Gerasimov 
> wrote:
>
>> Hello!
>>
>> The function Java_java_net_NetworkInterface_getByInetAddress0 may throw,
>> so after calling it we need to check if an exception is pending.
>>
>> Would you please help review a one-line fix?
>>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/
>>
>> Thanks in advance!
>>
>> --
>> With kind regards,
>> Ivan Gerasimov
>>
>>
>
> --
> Thanks,
> Vyom
>
>
> --
> With kind regards,
> Ivan Gerasimov
>
>

-- 
Thanks,
Vyom