Re: RFR 15 8243099: SO_INCOMING_NAPI_ID support

[Daniel Fuchs]

Hi Vladimir, Patrick,

The new tests look very good. There's just a small issue I noticed.
It's present in all the new tests - here is an example on 
AsynchronousSocketChannelNAPITest.java:


This code in @BeforeTest:

  63 try (var s = AsynchronousSocketChannel.open()) {
  64 if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID))
  65 throw new SkippedException("NAPI ID not supported 
on this system");

  66 }

will prevent this code in @Test

  77 } else {
  78 assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
  79 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, 42));
  80 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, null));

  81 }


from ever being run. So I would suggest moving that check (lines 77-81
and 92-96) into the @BeforeTest method as below, and simply assume
that the option is supported when you reach the @Test:

  60 @BeforeTest
  61 public void setup() throws IOException {
  62 IPSupport.throwSkippedExceptionIfNonOperational();
  63 try (var s = AsynchronousSocketChannel.open()) {
 if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID)) {
 assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, 42));
 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, null));


  65 throw new SkippedException("NAPI ID not supported 
on this system");

  66 }
  67 hostAddr = InetAddress.getLocalHost();
  68 }

Then the @Test can be simplified as follows:

  70 @Test
  71 public void testSetGetOptionSocketChannel() throws IOException {
  72 try (var sc = AsynchronousSocketChannel.open()) {
 assertEquals((int) sc.getOption(SO_INCOMING_NAPI_ID), 0);
 assertThrows(SE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, 42));
 assertThrows(IAE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, null));

  82 }
  83 }

best regards,

-- daniel

On 13/05/2020 21:50, Ivanov, Vladimir A wrote:

Thanks a lot. The updated webrev available as 
http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.13/

Thanks, Vladimir

Re: RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function

[Alan Bateman]

On 14/05/2020 14:09, Andrew Dinn wrote:

:
I was unaware that this was a spec requirement for static libs. Can you
point me at any discussion that explains why this is required and what
the function might be expected to do? or even tell me when it was added
to the spec? The macro appears to implement an empty function. Is there
any expectation/requirement that it could/should do something more than
that?

I see Bob's mail with a link to the JEP. You might also be looking for 
the "Support for Statically Linked Libraries" section of the JNI spec 
[1] that came from that JEP.


-Alan

[1] 
https://docs.oracle.com/en/java/javase/14/docs/specs/jni/invocation.html#support-for-statically-linked-libraries

Re: RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function

[Andrew Dinn]

Hi Alan,

On 12/05/2020 19:57, Alan Bateman wrote:
> Looks okay to me.

I am glad to hear that and also to see this fixed quickly (thanks Bob
for posting the patch). I'd like to see it backported to jdk11u (via
jdk14u, of course) if possible in order to allow stock jdk14u/11u to be
used to build GraalVM native images.

I was unaware that this was a spec requirement for static libs. Can you
point me at any discussion that explains why this is required and what
the function might be expected to do? or even tell me when it was added
to the spec? The macro appears to implement an empty function. Is there
any expectation/requirement that it could/should do something more than
that?

I am also wondering if this is the only such omission or if other libs
might require a corresponding JNI_OnLoad_extnet_xxx function.

regards,


Andrew Dinn
---

> On 12/05/2020 19:46, Bob Vandette wrote:
>> BUG:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8244853
>>
>> Please review this simple fix for JDK 15 which adds the required
>> JNI_OnLoad_extnet function to the libextnet.a
>> static library when it is built.
>>
>> the JDK 15 make static-libs-image target currently builds this static
>> library but it is not spec compliant and
>> causes the GraalVM native-image utility to fail generating executables
>> due to the lack of the OnLoad symbol.
>>
>>
>> CHANGE:
>>
>> diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> --- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> +++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> @@ -34,6 +34,11 @@
>>   #include "jdk_net_LinuxSocketOptions.h"
>>     /*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>> +/*
>>    * Class: jdk_net_LinuxSocketOptions
>>    * Method:    setQuickAck
>>    * Signature: (II)V
>> diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> --- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> +++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> @@ -32,6 +32,11 @@
>>   #include 
>>   #include "jni_util.h"
>>   +/*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>>   static jint socketOptionSupported(jint sockopt) {
>>   jint one = 1;
>>   jint rv, s;
>> diff --git
>> a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> --- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> +++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> @@ -32,6 +32,11 @@
>>   static int initialized = 0;
>>     /*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>> +/*
>>    * Class: jdk_net_SolarisSocketOptions
>>    * Method:    init
>>    * Signature: ()V
> 

-- 
regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill

Re: RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function

[Bob Vandette]

> On May 14, 2020, at 9:09 AM, Andrew Dinn  wrote:
> 
> Hi Alan,
> 
> On 12/05/2020 19:57, Alan Bateman wrote:
>> Looks okay to me.
> 
> I am glad to hear that and also to see this fixed quickly (thanks Bob
> for posting the patch). I'd like to see it backported to jdk11u (via
> jdk14u, of course) if possible in order to allow stock jdk14u/11u to be
> used to build GraalVM native images.
> 
> I was unaware that this was a spec requirement for static libs. Can you
> point me at any discussion that explains why this is required and what
> the function might be expected to do? or even tell me when it was added
> to the spec? The macro appears to implement an empty function. Is there
> any expectation/requirement that it could/should do something more than
> that?

The static library support was added in JDK 8 under JEP 178 
(https://openjdk.java.net/jeps/178 )
and integrated under https://bugs.openjdk.java.net/browse/JDK-8046168 
.

The macros that were added to jni_util.h are not part of the specification and 
were added
for the JDK build to use.   There are two forms of the macros.  DEF_JNI_OnLoad 
is used when
the onload function runs initialization logic and DEF_STATIC_JNI_OnLoad that 
adds
an empty function only when building the library statically.  Both macros 
append the library
name when building static forms of each library.

> 
> I am also wondering if this is the only such omission or if other libs
> might require a corresponding JNI_OnLoad_extnet_xxx function.

All JDK native libraries should have this support but I supposed it’s possible 
that new libraries have
been added since JDK 8 that are missing the DEF_STATIC_JNI_OnLoad.  I did a 
quick scan
of the sources when implementing this fix and did not see any bare JNI_OnLoad 
functions.

Bob.


> 
> regards,
> 
> 
> Andrew Dinn
> ---
> 
>> On 12/05/2020 19:46, Bob Vandette wrote:
>>> BUG:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8244853
>>> 
>>> Please review this simple fix for JDK 15 which adds the required
>>> JNI_OnLoad_extnet function to the libextnet.a
>>> static library when it is built.
>>> 
>>> the JDK 15 make static-libs-image target currently builds this static
>>> library but it is not spec compliant and
>>> causes the GraalVM native-image utility to fail generating executables
>>> due to the lack of the OnLoad symbol.
>>> 
>>> 
>>> CHANGE:
>>> 
>>> diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>>> b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>>> --- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>>> +++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>>> @@ -34,6 +34,11 @@
>>>   #include "jdk_net_LinuxSocketOptions.h"
>>> /*
>>> + * Declare library specific JNI_Onload entry if static build
>>> + */
>>> +DEF_STATIC_JNI_OnLoad
>>> +
>>> +/*
>>>* Class: jdk_net_LinuxSocketOptions
>>>* Method:setQuickAck
>>>* Signature: (II)V
>>> diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>>> b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>>> --- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>>> +++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>>> @@ -32,6 +32,11 @@
>>>   #include 
>>>   #include "jni_util.h"
>>>   +/*
>>> + * Declare library specific JNI_Onload entry if static build
>>> + */
>>> +DEF_STATIC_JNI_OnLoad
>>> +
>>>   static jint socketOptionSupported(jint sockopt) {
>>>   jint one = 1;
>>>   jint rv, s;
>>> diff --git
>>> a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>>> b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>>> --- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>>> +++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>>> @@ -32,6 +32,11 @@
>>>   static int initialized = 0;
>>> /*
>>> + * Declare library specific JNI_Onload entry if static build
>>> + */
>>> +DEF_STATIC_JNI_OnLoad
>>> +
>>> +/*
>>>* Class: jdk_net_SolarisSocketOptions
>>>* Method:init
>>>* Signature: ()V
>> 
> 
> -- 
> regards,
> 
> 
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill
> 

Re: 8244031: HttpClient should have more tests for HEAD requests

[Michael McMahon]

On 13/05/2020 15:07, Chris Hegarty wrote:

On 8 May 2020, at 16:46, Daniel Fuchs  wrote:

Hi,

Please find a fix for:

8244031: HttpClient should have more tests for HEAD requests
https://bugs.openjdk.java.net/browse/JDK-8244031

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8244031/webrev.00/

Seems like a comprehensive test.


The test was developed while investigating
https://bugs.openjdk.java.net/browse/JDK-8243126

Thanks for circling back and adding the additional test coverage.

-Chris.


+1

Michael

Re: RFR 8244652: Add test for non utf-8 response handling by websocket client

[Rahul]

Thanks Daniel
I would proceed with the push if there are no further comments.

On 13/05/2020, 12:39, "Daniel Fuchs"  wrote:

Hi Rahul,

That looks good to me thanks!

I am still teetering on whether we should also have altered the
specification of WSHandshakeException, and explicitly changed the
signature of WSHandshakeException::getResponse to return an
HttpResponse instead of an HttpResponse.

On the one hand - our implementation will now actually instantiate
an HttpResponse. On the other hand - it might not be fair
to actually require it in the specification.

Maybe Pavel will have some thoughts on the subject.

As for me - your changes to the test look good.

best regards

-- daniel

On 12/05/2020 20:05, Rahul wrote:
> Hello,
> 
> Request to have my fix reviewed for the issue:
> 
>JDK-8244652: Add test for non utf-8 response handling by websocket 
> client.
> 
> The test java.net.httpclient.websocket.WSHandshakeExceptionTest.java is 
> updated to test
> 
>that the websocket client handles invalid utf-8 sent by the websocket 
> server as part of the
> 
>websocket exception whenthe handshake is rejected by websocket server.
> 
>Issue:   https://bugs.openjdk.java.net/browse/JDK-8244652
> 
>Webrev: 
> http://cr.openjdk.java.net/~ryadav/webrev_8244652/webrev.00/index.html
> 
>   - rahul
> 

RE: RFR 15 8243099: SO_INCOMING_NAPI_ID support

[Viswanathan, Sandhya]

I updated the CSR wordings slightly to make it easier to read. If no 
objections, I plan to move it to "Proposed" today. 

https://bugs.openjdk.java.net/browse/JDK-8244858

Best Regards,
Sandhya

-Original Message-
From: net-dev  On Behalf Of Viswanathan, 
Sandhya
Sent: Tuesday, May 12, 2020 3:14 PM
To: Alan Bateman ; Ivanov, Vladimir A 
; OpenJDK Network Dev list 

Subject: RE: RFR 15 8243099: SO_INCOMING_NAPI_ID support


I have created the following draft CSR:
  https://bugs.openjdk.java.net/browse/JDK-8244858

Please review and let me know if I can move it to proposed.

Best Regards,
Sandhya

-Original Message-
From: net-dev  On Behalf Of Alan Bateman
Sent: Friday, May 08, 2020 12:10 AM
To: Ivanov, Vladimir A ; OpenJDK Network Dev list 

Subject: Re: RFR 15 8243099: SO_INCOMING_NAPI_ID support

On 07/05/2020 19:51, Ivanov, Vladimir A wrote:
> In my case for 2 servers with RHEL8.1 the NapiId was non-zero for the 
> DatagramSocket after the 'receive' call.
>
Thanks for checking. I tried the equivalent of RHEL7.6 and it consistently 
returns 0 for UDP sockets so they may be kernel differences that explain this.

I took the liberty of tweaking the javadoc to allow for a bit more flexibility 
as to reasons why the socket option value may be 0. This allows us to drop the 
distinction between connecting and listing sockets. If you are okay with this 
text then let's give it a day or two to see if there are other comments before 
Sandhya submits the CSR.

-Alan


     /**
  * Identifies the receive queue that the last incoming packet for the 
socket
  * was received on.
  *
  *  The value of this socket option is a positive {@code Integer} that
  * identifies a receive queue that the application can use to split the
  * incoming flows among threads based on the queue identifier. The value is
  * {@code 0} when the socket is not bound, a packet has not been received,
  * or more generally, when there is no receive queue to identify. 
The socket
  * option is supported by both stream-oriented and datagram-oriented
  * sockets.
  *
  *  The socket option is read-only and an attempt to set the socket 
option
  * will throw {@code SocketException}.
  *
  * @apiNote
  * Network devices may have multiple queues or channels to transmit and 
receive
  * network packets. The {@code SO_INCOMING_NAPI_ID} socket option provides 
a hint
  * to the application to indicate the receive queue on which an incoming 
socket
  * connection or packets for that connection are directed to. An 
application may
  * take advantage of this by handling all socket connections assigned to a
  * specific queue on one thread.
  *
  * @since 15
  */

RE: RFR 15 8243099: SO_INCOMING_NAPI_ID support

[Ivanov, Vladimir A]

Thanks a lot Daniel! I missed these double checks.
Updated webrev may be reviewed  as 
http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.14 
I use only one condition for the 'if' in the 'startup' method while kernel 
should support or not both types of sockets together.

 Thanks, Vladimir

-Original Message-
From: Daniel Fuchs  
Sent: Thursday, May 14, 2020 2:37 AM
To: Ivanov, Vladimir A ; Patrick Concannon 
; Alan Bateman ; OpenJDK 
Network Dev list 
Subject: Re: RFR 15 8243099: SO_INCOMING_NAPI_ID support

Hi Vladimir, Patrick,

The new tests look very good. There's just a small issue I noticed.
It's present in all the new tests - here is an example on
AsynchronousSocketChannelNAPITest.java:

This code in @BeforeTest:

   63 try (var s = AsynchronousSocketChannel.open()) {
   64 if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID))
   65 throw new SkippedException("NAPI ID not supported 
on this system");
   66 }

will prevent this code in @Test

   77 } else {
   78 assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
   79 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, 42));
   80 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, null));
   81 }


from ever being run. So I would suggest moving that check (lines 77-81 and 
92-96) into the @BeforeTest method as below, and simply assume that the option 
is supported when you reach the @Test:

   60 @BeforeTest
   61 public void setup() throws IOException {
   62 IPSupport.throwSkippedExceptionIfNonOperational();
   63 try (var s = AsynchronousSocketChannel.open()) {
  if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID)) {
  assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
  assertThrows(UOE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
42));
  assertThrows(UOE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
null));

   65 throw new SkippedException("NAPI ID not supported 
on this system");
   66 }
   67 hostAddr = InetAddress.getLocalHost();
   68 }

Then the @Test can be simplified as follows:

   70 @Test
   71 public void testSetGetOptionSocketChannel() throws IOException {
   72 try (var sc = AsynchronousSocketChannel.open()) {
  assertEquals((int) sc.getOption(SO_INCOMING_NAPI_ID), 0);
  assertThrows(SE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 42));
  assertThrows(IAE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
null));
   82 }
   83 }

best regards,

-- daniel

On 13/05/2020 21:50, Ivanov, Vladimir A wrote:
> Thanks a lot. The updated webrev available as 
> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.13/
> 
> Thanks, Vladimir