Re: RFR 8242999: http/2 client may not handle continuation frames correctly
Rahul, > On 24 Apr 2020, at 15:58, Rahul wrote: > > Hello, > > Request to have my fix reviewed for the issue: > JDK-8242999 : http/2 client may not handle continuation frames correctly. > > The fix updates jdk.internal.net.http.Stream.incoming(Http2Frame frame) to > handle > the scenario where a continuation with `END_HEADERS` may appear after a > header > frame, this was not being handled earlier. > > Issue: https://bugs.openjdk.java.net/browse/JDK-8242999 > Webrev: > http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.00/ > Looks good. Can you please update the comment in the test. Suggest: * A function that returns a list of 1) a HEADERS frame WITH END_STREAM *( AND an empty payload ), and 2) two CONTINUATION frames, THE FIRST * is empty and THE SECOND with headers AND END_HEADERS. -Chris.
Re: RFR 8242999: http/2 client may not handle continuation frames correctly
Hi Rahul, That looks very good! Thanks for taking that on. Not related to your fix, but could you update the code that creates the HttpClient to explicitly require no proxy? I suggest to update: 166 client = HttpClient.newBuilder().sslContext(sslContext).build(); to: client = HttpClient.newBuilder() .proxy(HttpClient.Builder.NO_PROXY) .sslContext(sslContext) .build(); Not explicitly requiring NO_PROXY has been a source of instability on certain environments - as it might pick up system proxies. best regards, -- daniel On 24/04/2020 15:58, Rahul wrote: Hello, Request to have my fix reviewed for the issue: JDK-8242999 : http/2 client may not handle continuation frames correctly. The fix updates jdk.internal.net.http.Stream.incoming(Http2Frame frame) to handle the scenario where a continuation with `END_HEADERS` may appear after a header frame, this was not being handled earlier. Issue: https://bugs.openjdk.java.net/browse/JDK-8242999 Webrev: http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.00/ --rahul
Re: RFR 8242999: http/2 client may not handle continuation frames correctly
Thanks for the review comments. I have updated the webrev : http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.01/ On 27/04/2020, 12:42, "Daniel Fuchs" wrote: Hi Rahul, That looks very good! Thanks for taking that on. Not related to your fix, but could you update the code that creates the HttpClient to explicitly require no proxy? I suggest to update: 166 client = HttpClient.newBuilder().sslContext(sslContext).build(); to: client = HttpClient.newBuilder() .proxy(HttpClient.Builder.NO_PROXY) .sslContext(sslContext) .build(); Not explicitly requiring NO_PROXY has been a source of instability on certain environments - as it might pick up system proxies. best regards, -- daniel On 24/04/2020 15:58, Rahul wrote: > Hello, > > Request to have my fix reviewed for the issue: > > JDK-8242999 : http/2 client may not handle continuation frames correctly. > > The fix updates jdk.internal.net.http.Stream.incoming(Http2Frame frame) > to handle > > the scenario where a continuation with `END_HEADERS` may appear after a > header > > frame, this was not being handled earlier. > > Issue: https://bugs.openjdk.java.net/browse/JDK-8242999 > > Webrev: > http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.00/ > >--rahul >
Re: RFR 8242999: http/2 client may not handle continuation frames correctly
On 27/04/2020 14:04, Rahul wrote: Thanks for the review comments. I have updated the webrev : http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.01/ Thanks Rahul! LGTM. -- daniel
Re: RFR 8242999: http/2 client may not handle continuation frames correctly
Rahul, > On 27 Apr 2020, at 14:04, Rahul wrote: > > Thanks for the review comments. > I have updated the webrev : > http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.01/ Seems you took my suggestion too literally ;-) I capitalised some of the text to clearly demarcate it as NEW, rather than the exact text. So that would be: * A function that returns a list of 1) a HEADERS frame with END_STREAM *( and an empty payload ), and 2) two CONTINUATION frames, the first * is empty, and the second contains headers and the END_HEADERS flag. No need for another webrev. -Chris
Re: RFR 8242999: http/2 client may not handle continuation frames correctly
Hi Chris, Thank you for letting know, will update. -- rahul From: Chris Hegarty Date: Monday 27 April 2020 at 17:00 To: Rahul , OpenJDK Network Dev list Subject: Re: RFR 8242999: http/2 client may not handle continuation frames correctly Rahul, On 27 Apr 2020, at 14:04, Rahul wrote: Thanks for the review comments. I have updated the webrev : http://cr.openjdk.java.net/~pconcannon/rayayada/8242999/webrevs/webrev.01/ Seems you took my suggestion too literally ;-) I capitalised some of the text to clearly demarcate it as NEW, rather than the exact text. So that would be: * A function that returns a list of 1) a HEADERS frame with END_STREAM *( and an empty payload ), and 2) two CONTINUATION frames, the first * is empty, and the second contains headers and the END_HEADERS flag. No need for another webrev. -Chris
RE: RFR 15 8243099: Adding ADQ support to JDK
Thanks for the comments. The webrev was updated: http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.02/ Only the code was changed here. Changes in the documentation will be discussed separately. Changes: 1. The LinuxSocketOptions.c was updated according to Vyom comment. Note, the 'socketOptionSupported' implemented through the "setsockopt" call that always return error for the read only properties. It was updated to use 'getsockopt'. The tests set 'test/jdk/java/net test/jdk/java/nio/channels test/jdk/javax/net test/jdk/jdk/net test/jdk/sun/net' have same run results on RHEL7.7 as clean jdk/jdk workspace. Please, notify me if testing should be extended. 2. The 'IncomingNapiIdSupported0' was renamed to the 'incomingNapiIdSupported0.' 3. The test 'PrintSupportedOptions' was updated to use Set for read only options. 4. The test ExtOptionNAPITest.java was added. Now the test 'ExtOptionTest.java' do nothing with napi id. Thanks, Vladimir -Original Message- From: Alan Bateman Sent: Friday, April 24, 2020 12:09 AM To: Ivanov, Vladimir A ; OpenJDK Network Dev list Subject: Re: RFR 15 8243099: Adding ADQ support to JDK On 23/04/2020 20:11, Ivanov, Vladimir A wrote: > Thanks a lot to Chris and Alan for detailed comments. > The updated version of patch available at > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.01/ > > Changes: > 1. in native code the common pattern was used for the 'getsockopt' call. > 2. condition to define SO_INCOMING_NAPI_ID was added. > 3. the DatagarmSocket was added to the ExtOptionTest 4. testing on my > side was extended to the subset 'test/jdk/java/net test/jdk/java/nio/channels > test/jdk/javax/net test/jdk/jdk/net test/jdk/sun/net'. > Results are same for the patched and non-patched builds on the RHEL7.7 OS. > Tests test/jdk/java/net/SocketOption/AfterClose.java and > test/jdk/java/nio/channels/etc/PrintSupportedOptions.java were updated to > support read only properties. > 5. description for the NAPI_ID was updated 6. the > UnsupportedOperationException was added to the 'setOption' call for the > 'SO_INCOMING_NAPI_ID'. > (Dropping core-libs-dev as net-dev is the more appropriate list for this area). Thanks for the update. The updated javadoc looks better but will need a few iterations. It would be good if it could start by explaining what the value of the socket option is, e.g. "The value of this socket option is an Integer representing the network device queue ...". I think it needs to be clearer on the types of sockets that support this option, does it support both stream-oriented and datagram-oriented sockets? Does it return a value when invoked on a ServerSocketChannel? Instead of @throws, the text will need to that attempting to set the socket option will cause UOE to be thrown. Once the javadoc is agreed then the CSR can be submitted and the code review can continue in parallel. The implementation changes mostly look okay although IncomingNapiIdSupported0 should probably be renamed to start with lower case "i". Also someone might need to check that you can create an IPv4 socket when a system is configured as an IPv6-only system. Tests. I think the the new tests in ExtOptionTest will need closer examination as I can't tell how reliable they are. It might be that it needs a completely new test. The update to the NIO PrintSupportedOptions test define READ_ONLY_OPTS as Set. -Alan
RE: RFR 15 8243099: Adding ADQ support to JDK
The changes for ADq doc update that includes explanation for this technology: /** * ADQ (Application Device Queues) is an open technology to help address network traffic challenges by improving application * throughput and latency, and, most importantly, by enabling greater predictability in application response times by creating * application specific traffic queuing and steering. * * Modern Network Interface Card (NIC) devices have multiple queues or channels to transmit and receive Network packets. ADQ * lets each software application reserve subset of these device queues, so that all the traffic, and only the traffic, belonging to the * application is directed to those set of queues avoiding sharing or competing for network resources with other applications in the * system. * * System Administrator allocates Network resources to an application in a multi-application environment including set of device * queues (Number of Tx/RX queue pairs), dedicated for application traffic. In addition, QoS attributes can be applied to these * Application Queues such as Network bandwidth limits & Traffic priority. Device then assigns the incoming application connections * to these set of queues by defined policies such as round robin. ADQ provides hints to applications, by means of a new socket option * SO_INCOMING_NAPI_ID, to indicate the device queue from the set of assigned queues, to which an incoming socket connection and * packets for that connection are directed to. * * For a multi-threaded application to take advantage of these dedicated queues, application needs ability to query which queue a * connection is assigned to, so all socket connections from a specific device queue can be serviced by a single application thread. * Application then may utilize busy polling to receive and transmit the network packets, minimizing system interrupts and context * switches to better align overall system resources to applications. This helps in improved predictability, reduce latency and improve * throughput for ADQ enabled Application. * * The value of this SO_INCOMING_NAPI_ID socket option is an Integer, uniquely representing the network device queue on which the * last packet of the socket connection is received on. This option is available for both stream-oriented and datagram-oriented sockets * on both client and server sockets. The value returned for this socket option will be zero until the socket is connected and has received * a network packet, at which point it will be a non-zero integer value. This is a read only option. Attempting to set the socket option will * cause UOE to be thrown. */ Thanks, Vladimir -Original Message- From: Alan Bateman Sent: Friday, April 24, 2020 12:09 AM To: Ivanov, Vladimir A ; OpenJDK Network Dev list Subject: Re: RFR 15 8243099: Adding ADQ support to JDK On 23/04/2020 20:11, Ivanov, Vladimir A wrote: > Thanks a lot to Chris and Alan for detailed comments. > The updated version of patch available at > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.01/ > > Changes: > 1. in native code the common pattern was used for the 'getsockopt' call. > 2. condition to define SO_INCOMING_NAPI_ID was added. > 3. the DatagarmSocket was added to the ExtOptionTest 4. testing on my > side was extended to the subset 'test/jdk/java/net test/jdk/java/nio/channels > test/jdk/javax/net test/jdk/jdk/net test/jdk/sun/net'. > Results are same for the patched and non-patched builds on the RHEL7.7 OS. > Tests test/jdk/java/net/SocketOption/AfterClose.java and > test/jdk/java/nio/channels/etc/PrintSupportedOptions.java were updated to > support read only properties. > 5. description for the NAPI_ID was updated 6. the > UnsupportedOperationException was added to the 'setOption' call for the > 'SO_INCOMING_NAPI_ID'. > (Dropping core-libs-dev as net-dev is the more appropriate list for this area). Thanks for the update. The updated javadoc looks better but will need a few iterations. It would be good if it could start by explaining what the value of the socket option is, e.g. "The value of this socket option is an Integer representing the network device queue ...". I think it needs to be clearer on the types of sockets that support this option, does it support both stream-oriented and datagram-oriented sockets? Does it return a value when invoked on a ServerSocketChannel? Instead of @throws, the text will need to that attempting to set the socket option will cause UOE to be thrown. Once the javadoc is agreed then the CSR can be submitted and the code review can continue in parallel. The implementation changes mostly look okay although IncomingNapiIdSupported0 should probably be renamed to start with lower case "i". Also someone might need to check that you can create an IPv4 socket when a system is configured as an IPv6-only system. Tests. I think the the new tests in ExtOptionTest will need closer examination as I can't tell h
Re: RFR 15 8243099: Adding ADQ support to JDK
Hi Vladimir, changes looks much better now, i still see that function(IncomingNapiIdSupported) & variable(IncomingNapiIdOptSupported) does not follow Java naming convention. private static final boolean IncomingNapiIdOptSupported =+ platformSocketOptions.IncomingNapiIdSupported(); Thanks, Vyom On Tue, Apr 28, 2020 at 3:36 AM Ivanov, Vladimir A < vladimir.a.iva...@intel.com> wrote: > Thanks for the comments. The webrev was updated: > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.02/ > > Only the code was changed here. Changes in the documentation will be > discussed separately. > > Changes: > 1. The LinuxSocketOptions.c was updated according to Vyom comment. > Note, the 'socketOptionSupported' implemented through the "setsockopt" > call that always return error for the read only properties. > It was updated to use 'getsockopt'. The tests set 'test/jdk/java/net > test/jdk/java/nio/channels test/jdk/javax/net test/jdk/jdk/net > test/jdk/sun/net' > have same run results on RHEL7.7 as clean jdk/jdk workspace. Please, > notify me if testing should be extended. > > 2. The 'IncomingNapiIdSupported0' was renamed to the > 'incomingNapiIdSupported0.' > > 3. The test 'PrintSupportedOptions' was updated to use Set for > read only options. > > 4. The test ExtOptionNAPITest.java was added. Now the test > 'ExtOptionTest.java' do nothing with napi id. > > Thanks, Vladimir > > -Original Message- > From: Alan Bateman > Sent: Friday, April 24, 2020 12:09 AM > To: Ivanov, Vladimir A ; OpenJDK Network Dev > list > Subject: Re: RFR 15 8243099: Adding ADQ support to JDK > > On 23/04/2020 20:11, Ivanov, Vladimir A wrote: > > Thanks a lot to Chris and Alan for detailed comments. > > The updated version of patch available at > > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.01/ > > > > Changes: > > 1. in native code the common pattern was used for the 'getsockopt' call. > > 2. condition to define SO_INCOMING_NAPI_ID was added. > > 3. the DatagarmSocket was added to the ExtOptionTest 4. testing on my > > side was extended to the subset 'test/jdk/java/net > test/jdk/java/nio/channels test/jdk/javax/net test/jdk/jdk/net > test/jdk/sun/net'. > > Results are same for the patched and non-patched builds on the RHEL7.7 > OS. > > Tests test/jdk/java/net/SocketOption/AfterClose.java and > > test/jdk/java/nio/channels/etc/PrintSupportedOptions.java were updated > to support read only properties. > > 5. description for the NAPI_ID was updated 6. the > > UnsupportedOperationException was added to the 'setOption' call for the > 'SO_INCOMING_NAPI_ID'. > > > (Dropping core-libs-dev as net-dev is the more appropriate list for this > area). > > Thanks for the update. > > The updated javadoc looks better but will need a few iterations. It would > be good if it could start by explaining what the value of the socket option > is, e.g. "The value of this socket option is an Integer representing the > network device queue ...". I think it needs to be clearer on the types of > sockets that support this option, does it support both stream-oriented and > datagram-oriented sockets? Does it return a value when invoked on a > ServerSocketChannel? Instead of @throws, the text will need to that > attempting to set the socket option will cause UOE to be thrown. Once the > javadoc is agreed then the CSR can be submitted and the code review can > continue in parallel. > > The implementation changes mostly look okay although > IncomingNapiIdSupported0 should probably be renamed to start with lower > case "i". Also someone might need to check that you can create an IPv4 > socket when a system is configured as an IPv6-only system. > > Tests. I think the the new tests in ExtOptionTest will need closer > examination as I can't tell how reliable they are. It might be that it > needs a completely new test. The update to the NIO PrintSupportedOptions > test define READ_ONLY_OPTS as Set. > > -Alan > -- Thanks, Vyom
