Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v3]
On Thu, 12 May 2022 11:54:55 GMT, Conor Cleary wrote: >> **Issue** >> A recent fix for >> [JDK-8263031](https://bugs.openjdk.java.net/browse/JDK-8263031), which >> addressed the httpclient being unable to properly process Http/2 PushPromise >> frames followed by continuations caused intermittent failures of the test. >> This was cause by two seperate problems. >> >> - Firstly, on the client side, `Http2Connection.processFrame` did not a >> check for whether or not a Continuation was to be expected early on in the >> method resulting in frames other than the expected Continuation Frame being >> incorrectly processed. In Http/2, when a Continuation Frame is expected, no >> other type of frame should be processed without a connection error of type >> PROTOCOL_ERROR being thrown. >> - Secondly, `Http2TestConnection.handlePush` had no guards around response >> headers and data frames being sent before a continuation is sent which >> resulted in the intermittent nature of this timeout. >> >> **Proposed Solution** >> The test class `OutgoingPushPromise` was modified to contain a list of >> Continuation Frames as required rather than the Continuations being >> scheduled to send in the test itself. This prevents the situation of >> unexpected frames being sent before a Continuation Frame was meant to >> arrive. >> >> In addition to the above, Http2Connection was modified to ensure that a >> connection error of type PROTOCOL_ERROR is thrown if a frame other than a >> Continuation arrives at the client unexpectedly. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8284585: Added test case to verify fix Test seems stable with latest changes after multiple runs, will integrate. - PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v7]
On Thu, 12 May 2022 12:23:34 GMT, Jaikiran Pai wrote: >> Will await further review for a short while and then integrate if approved. > >> Will await further review for a short while and then integrate if approved. > > Hello Conor, looking at the latest state of the PR, I think you might have > missed Daniel's review comment > https://github.com/openjdk/jdk/pull/8017#discussion_r866857608 which I think > has caught an unintentional change in this PR. Fixing now, was an ommision on my part. Thank you for catching it @jaikiran - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows The CSR for this has been approved. If the current state of the PR looks good, then I plan to integrate this later today. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]
> **Issue** > When using the `HttpClient.send()` to send a GET request created using the > `HttpRequest.newBuilder()`, a `Content-length: 0` header is set. This > behaviour causes issues with many services as a body related header is > usually not expected to be included with a GET request. > > **Solution** > `Http1Request.java` was modified so that when the request method is a GET, a > `Content-length` header is not added to the request. However, if a developer > chooses to include a body in a GET request (though it is generally considered > bad practice), a `Content-length` header with the appropriate value will be > added. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8283544: Added in missing case - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/f6efc915..28edba8c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=06-07 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8017/head:pull/8017 PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6690
Integrated: 8284585: PushPromiseContinuation test fails intermittently in timeout
On Tue, 3 May 2022 15:00:51 GMT, Conor Cleary wrote: > **Issue** > A recent fix for > [JDK-8263031](https://bugs.openjdk.java.net/browse/JDK-8263031), which > addressed the httpclient being unable to properly process Http/2 PushPromise > frames followed by continuations caused intermittent failures of the test. > This was cause by two seperate problems. > > - Firstly, on the client side, `Http2Connection.processFrame` did not a check > for whether or not a Continuation was to be expected early on in the method > resulting in frames other than the expected Continuation Frame being > incorrectly processed. In Http/2, when a Continuation Frame is expected, no > other type of frame should be processed without a connection error of type > PROTOCOL_ERROR being thrown. > - Secondly, `Http2TestConnection.handlePush` had no guards around response > headers and data frames being sent before a continuation is sent which > resulted in the intermittent nature of this timeout. > > **Proposed Solution** > The test class `OutgoingPushPromise` was modified to contain a list of > Continuation Frames as required rather than the Continuations being scheduled > to send in the test itself. This prevents the situation of unexpected frames > being sent before a Continuation Frame was meant to arrive. > > In addition to the above, Http2Connection was modified to ensure that a > connection error of type PROTOCOL_ERROR is thrown if a frame other than a > Continuation arrives at the client unexpectedly. This pull request has now been integrated. Changeset: 65da38d8 Author:Conor Cleary Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/65da38d844760f7d17a143f8b4d5e25ea0144e27 Stats: 100 lines in 4 files changed: 83 ins; 6 del; 11 mod 8284585: PushPromiseContinuation test fails intermittently in timeout Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows Thank you Daniel and Michael for the constant inputs and reviews on this one. - PR: https://git.openjdk.java.net/jdk/pull/6690
Integrated: 8209137: Add ability to bind to specific local address to HTTP client
On Fri, 3 Dec 2021 06:15:31 GMT, Jaikiran Pai wrote: > This change proposes to implement the enhancement noted in > https://bugs.openjdk.java.net/browse/JDK-8209137. > > The change introduces a new API to allow applications to build a > `java.net.http.HTTPClient` configured with a specific local address that will > be used while creating `Socket`(s) for connections. This pull request has now been integrated. Changeset: f4258a50 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/f4258a50e0f65ab9c375b9ee79f31de98d872550 Stats: 598 lines in 11 files changed: 589 ins; 5 del; 4 mod 8209137: Add ability to bind to specific local address to HTTP client Reviewed-by: dfuchs, michaelm - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]
On Mon, 16 May 2022 08:59:43 GMT, Conor Cleary wrote: >> **Issue** >> When using the `HttpClient.send()` to send a GET request created using the >> `HttpRequest.newBuilder()`, a `Content-length: 0` header is set. This >> behaviour causes issues with many services as a body related header is >> usually not expected to be included with a GET request. >> >> **Solution** >> `Http1Request.java` was modified so that when the request method is a GET, a >> `Content-length` header is not added to the request. However, if a developer >> chooses to include a body in a GET request (though it is generally >> considered bad practice), a `Content-length` header with the appropriate >> value will be added. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8283544: Added in missing case LGTM. If the test is stable you can integrate. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]
On Mon, 16 May 2022 08:59:43 GMT, Conor Cleary wrote: >> **Issue** >> When using the `HttpClient.send()` to send a GET request created using the >> `HttpRequest.newBuilder()`, a `Content-length: 0` header is set. This >> behaviour causes issues with many services as a body related header is >> usually not expected to be included with a GET request. >> >> **Solution** >> `Http1Request.java` was modified so that when the request method is a GET, a >> `Content-length` header is not added to the request. However, if a developer >> chooses to include a body in a GET request (though it is generally >> considered bad practice), a `Content-length` header with the appropriate >> value will be added. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8283544: Added in missing case Thank you for the changes, Conor. The latest state looks fine to me. - Marked as reviewed by jpai (Committer). PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - review update > - update some nits > - PR ready > - Initial There is too much grey area. It says the src buffer maybe modified, which one could interpret it cannot be a read-only, but that would still need clarification to explicitly say "no read only buffers". And other than these internal 'in-place' crypto reason, there is no API reason to not allow read-only buffers as input. I did think about closing the CSR as the text was already there about the src buffer, even thought it was using a different term. But I felt strongly enough that I wanted to prevent that confusion in the future. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Mon, 16 May 2022 21:08:48 GMT, Anthony Scarpino
wrote:
> There is too much grey area. It says the src buffer maybe modified, which one
> could interpret it cannot be a read-only, but that would still need
> clarification to explicitly say "no read only buffers". And other than these
> internal 'in-place' crypto reason, there is no API reason to not allow
> read-only buffers as input. I did think about closing the CSR as the text was
> already there about the src buffer, even thought it was using a different
> term. But I felt strongly enough that I wanted to prevent that confusion in
> the future.
I think it is good to make the clarification with a CSR.
As the spec says, "The inbound network buffer, {@code src}, may be modified",
applications cannot assume that the inbound network buffer cannot be modified
(read-only) any longer. It does not grant that an application will work with
read-only inbound buffers for another JSSE provider. As a result, an
application that want to support read-only buffers may also need to support
non-read-only buffers. As makes it more complicated in both application layers
and the JSSE implementation layer. It may be simple that applications and JSSE
implementation codes only need to handle non-read-only buffers.
Just my $0.02. I will let you and Brad for the final decision.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
