RFR: 8286962: java/net/httpclient/ServerCloseTest.java failed once with ConnectException
**Issue** A failure in this test was observed whereby an unexpected connection from a client that was not created by the test caused the test to unsucessfully complete. **Solution** When a connection is accepted by the ServerSocket, some verification of the Request URI and of the Request Path is conducted. If the client connection is found to be invalid or external to the test, the connection is closed. The ServerSocket will continue to accept new connections until the correct parameters are met. Once a valid connection is accepted, the test behaves exactly as it previously did. - Commit messages: - 8286962: java/net/httpclient/ServerCloseTest.java failed once with ConnectException Changes: https://git.openjdk.org/jdk/pull/9155/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=9155&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8286962 Stats: 89 lines in 1 file changed: 37 ins; 31 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/9155.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9155/head:pull/9155 PR: https://git.openjdk.org/jdk/pull/9155
Integrated: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
On Wed, 8 Jun 2022 18:29:10 GMT, Conor Cleary wrote: > **Issue** > It was observed that when the httpclient sends a POST request with the > `Expect: 100 Continue` header set and the server replies with a response code > `417 Expectation Failed` that the httpclient hangs indefinitely when the > version of Http used is HTTP/2. However, it was also seen that the issue > persisted with HTTP/1_1 with the same usage. > > This was caused by an implementation in ExchangeImpl that resulted in two > calls to readBodyAsync() in this case, where the second call causes the > indefinite hanging (as if there was a respomse body, it has already been > read). > > **Solution** > When ExchangeImpl::expectContinue() detects that a response code 417 is > received, two things occur. Firstly, a flag is set which ensures that the > connection is closed locally in this case. Secondly, the response is returned > to the client as a failed future, A unit test was added to ensure that this > usage of the httpclient does not cause hanging. This pull request has now been integrated. Changeset: 26714431 Author:Conor Cleary Committer: Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/267144311c96109421b897b359c155a963661d31 Stats: 329 lines in 6 files changed: 326 ins; 0 del; 3 mod 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 Reviewed-by: dfuchs, jpai - PR: https://git.openjdk.org/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v4]
> **Issue** > It was observed that when the httpclient sends a POST request with the > `Expect: 100 Continue` header set and the server replies with a response code > `417 Expectation Failed` that the httpclient hangs indefinitely when the > version of Http used is HTTP/2. However, it was also seen that the issue > persisted with HTTP/1_1 with the same usage. > > This was caused by an implementation in ExchangeImpl that resulted in two > calls to readBodyAsync() in this case, where the second call causes the > indefinite hanging (as if there was a respomse body, it has already been > read). > > **Solution** > When ExchangeImpl::expectContinue() detects that a response code 417 is > received, two things occur. Firstly, a flag is set which ensures that the > connection is closed locally in this case. Secondly, the response is returned > to the client as a failed future, A unit test was added to ensure that this > usage of the httpclient does not cause hanging. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8286171: Updated comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/9093/files - new: https://git.openjdk.java.net/jdk/pull/9093/files/dc5fa264..28fff3b3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/9093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9093/head:pull/9093 PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v3]
On Thu, 9 Jun 2022 11:49:52 GMT, Jaikiran Pai wrote:
>> Conor Cleary has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8286171: Package-protected access for method
>> - 8286171: Added checks for correct response codes
>
> test/jdk/java/net/httpclient/ExpectContinueTest.java line 102:
>
>> 100: // Due to limitations of the above Http1 Server, a manual
>> approach is taken to test the hanging with the
>> 101: // httpclient using Http1 so that the correct response header
>> can be returned for the test case
>> 102: http1HangServer = new Http1HangServer(saHang);
>
> It took me a while to understand why we have special server implementation
> for this. The comment helped :) It reminded me about the `ServerImpl` used by
> the `HttpServer`, which unconditionally sends a `100` response code when it
> sees a `Expect: 100-continue` in the request header.
Yep, thats spot on. That is the exact case which made it necessary to include
our own Server implementation for the HTTP/1 417 scenario.
> test/jdk/java/net/httpclient/ExpectContinueTest.java line 131:
>
>> 129: @Override
>> 130: public void handle(HttpTestExchange exchange) throws
>> IOException {
>> 131: try (InputStream is = exchange.getRequestBody();
>
> Should this also be doing something similar to what the `PostHandler` does
> for HTTP2 versioned request?
I don't think that is necessary here as in both HTTP/1 and HTTP/2 cases, the
flow is exactly the same. The case you mentioned was due to the HTTP/1 server
emitting a 100 response code in its implementation without it being explicitly
specified in the handler.
> test/jdk/java/net/httpclient/ExpectContinueTest.java line 264:
>
>> 262:
>> 263: HttpRequest getRequest = HttpRequest.newBuilder(getUri)
>> 264: .GET()
>
> Is this missing an `.expectContinue(true)` here?
Its not needed for a GET request as the client is not sending a body in that
case. In this test the GET serves only to complete the HTTP/2 upgrade before
testing with a POST request.
-
PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v2]
On Thu, 9 Jun 2022 11:09:23 GMT, Conor Cleary wrote: >> Yes - good catch! > > Good suggestion, no need for it to be public. Will do. I made this change but added protected by mistake instead of just removing the modifier. Will fix in subsequent change - PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v2]
On Thu, 9 Jun 2022 11:06:11 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java
>> line 424:
>>
>>> 422: }
>>> 423:
>>> 424: public void closeWhenFinished() {
>>
>> Hello Conor, do you think it might be better if we make this package private
>> access instead of `public`?
>
> Yes - good catch!
Good suggestion, no need for it to be public. Will do.
-
PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v3]
> **Issue** > It was observed that when the httpclient sends a POST request with the > `Expect: 100 Continue` header set and the server replies with a response code > `417 Expectation Failed` that the httpclient hangs indefinitely when the > version of Http used is HTTP/2. However, it was also seen that the issue > persisted with HTTP/1_1 with the same usage. > > This was caused by an implementation in ExchangeImpl that resulted in two > calls to readBodyAsync() in this case, where the second call causes the > indefinite hanging (as if there was a respomse body, it has already been > read). > > **Solution** > When ExchangeImpl::expectContinue() detects that a response code 417 is > received, two things occur. Firstly, a flag is set which ensures that the > connection is closed locally in this case. Secondly, the response is returned > to the client as a failed future, A unit test was added to ensure that this > usage of the httpclient does not cause hanging. Conor Cleary has updated the pull request incrementally with two additional commits since the last revision: - 8286171: Package-protected access for method - 8286171: Added checks for correct response codes - Changes: - all: https://git.openjdk.java.net/jdk/pull/9093/files - new: https://git.openjdk.java.net/jdk/pull/9093/files/ea059a21..dc5fa264 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=01-02 Stats: 10 lines in 2 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/9093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9093/head:pull/9093 PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v2]
> **Issue** > It was observed that when the httpclient sends a POST request with the > `Expect: 100 Continue` header set and the server replies with a response code > `417 Expectation Failed` that the httpclient hangs indefinitely when the > version of Http used is HTTP/2. However, it was also seen that the issue > persisted with HTTP/1_1 with the same usage. > > This was caused by an implementation in ExchangeImpl that resulted in two > calls to readBodyAsync() in this case, where the second call causes the > indefinite hanging (as if there was a respomse body, it has already been > read). > > **Solution** > When ExchangeImpl::expectContinue() detects that a response code 417 is > received, two things occur. Firstly, a flag is set which ensures that the > connection is closed locally in this case. Secondly, the response is returned > to the client as a failed future, A unit test was added to ensure that this > usage of the httpclient does not cause hanging. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8286171: Added teardown method and comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/9093/files - new: https://git.openjdk.java.net/jdk/pull/9093/files/ce847b45..ea059a21 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=00-01 Stats: 41 lines in 3 files changed: 22 ins; 2 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/9093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9093/head:pull/9093 PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
On Wed, 8 Jun 2022 18:29:10 GMT, Conor Cleary wrote:
> **Issue**
> It was observed that when the httpclient sends a POST request with the
> `Expect: 100 Continue` header set and the server replies with a response code
> `417 Expectation Failed` that the httpclient hangs indefinitely when the
> version of Http used is HTTP/2. However, it was also seen that the issue
> persisted with HTTP/1_1 with the same usage.
>
> This was caused by an implementation in ExchangeImpl that resulted in two
> calls to readBodyAsync() in this case, where the second call causes the
> indefinite hanging (as if there was a respomse body, it has already been
> read).
>
> **Solution**
> When ExchangeImpl::expectContinue() detects that a response code 417 is
> received, two things occur. Firstly, a flag is set which ensures that the
> connection is closed locally in this case. Secondly, the response is returned
> to the client as a failed future, A unit test was added to ensure that this
> usage of the httpclient does not cause hanging.
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 306:
> 304: @Override
> 305: void expectContinueFailed(int rcode) {
> 306: // TODO: add comment
Will add in a missing comment here!
-
PR: https://git.openjdk.java.net/jdk/pull/9093
RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
**Issue** It was observed that when the httpclient sends a POST request with the `Expect: 100 Continue` header set and the server replies with a response code `417 Expectation Failed` that the httpclient hangs indefinitely when the version of Http used is HTTP/2. However, it was also seen that the issue persisted with HTTP/1_1 with the same usage. This was caused by an implementation in ExchangeImpl that resulted in two calls to readBodyAsync() in this case, where the second call causes the indefinite hanging (as if there was a respomse body, it has already been read). **Solution** When ExchangeImpl::expectContinue() detects that a response code 417 is received, two things occur. Firstly, a flag is set which ensures that the connection is closed locally in this case. Secondly, the response is returned to the client as a failed future, A unit test was added to ensure that this usage of the httpclient does not cause hanging. - Commit messages: - 8286171: Added bug id - 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 Changes: https://git.openjdk.java.net/jdk/pull/9093/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=9093&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8286171 Stats: 302 lines in 6 files changed: 299 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/9093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9093/head:pull/9093 PR: https://git.openjdk.java.net/jdk/pull/9093
Integrated: 8283544: HttpClient GET method adds Content-Length: 0 header
On Tue, 29 Mar 2022 15:44:58 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. This pull request has now been integrated. Changeset: 6a770932 Author:Conor Cleary Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/6a7709320d28d8e1593b113fdf39ab583fca3687 Stats: 248 lines in 2 files changed: 234 ins; 6 del; 8 mod 8283544: HttpClient GET method adds Content-Length: 0 header Reviewed-by: dfuchs, jpai - PR: https://git.openjdk.java.net/jdk/pull/8017
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. Thanks @jaikiran, will integrate now! - PR: https://git.openjdk.java.net/jdk/pull/8017
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: 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: 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: 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 Fri, 6 May 2022 13:46:41 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: Removed unecessary control flow Will await further review for a short while and then integrate if approved. - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v2]
On Thu, 5 May 2022 13:37:08 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: New constructor & minor improvements Seeking additonal review from any watchers if possible - PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v3]
> **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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8518/files - new: https://git.openjdk.java.net/jdk/pull/8518/files/24e702a8..64fa937f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=01-02 Stats: 81 lines in 3 files changed: 58 ins; 9 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8518/head:pull/8518 PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v7]
> **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: Removed unecessary control flow - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/83c6ca6e..f6efc915 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=05-06 Stats: 22 lines in 2 files changed: 7 ins; 15 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: 8283544: HttpClient GET method adds Content-Length: 0 header [v6]
> **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: Improved logging, drain input stream - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/c1ef7d29..83c6ca6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=04-05 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 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: 8284585: PushPromiseContinuation test fails intermittently in timeout [v2]
> **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: New constructor & minor improvements - Changes: - all: https://git.openjdk.java.net/jdk/pull/8518/files - new: https://git.openjdk.java.net/jdk/pull/8518/files/cb6c189b..24e702a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=00-01 Stats: 48 lines in 4 files changed: 18 ins; 16 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8518/head:pull/8518 PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout
On Wed, 4 May 2022 11:17:32 GMT, Daniel Fuchs 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. > > test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 56: > >> 54: continuations = new LinkedList<>(); >> 55: continuations.add(cf); >> 56: } > > I would suggest adding a constructor that takes a list of > continuations/frames instead. This way you could have an immutable list (use > List.of/List.copyOf) Ah, good suggestion! - PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout
On Wed, 4 May 2022 11:13:26 GMT, Daniel Fuchs 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.
>
> test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java line
> 949:
>
>> 947: pp.streamid(op.parentStream);
>> 948: writeFrame(pp);
>> 949: if (pp.getFlags() != HeadersFrame.END_HEADERS &&
>> op.hasContinuations()) {
>
> Maybe you could just drop `op.hasContinuations()`
I had it like this to account for a case where a test could incorrectly add
continuations to an OutgoingPushPromise without correctly setting the
END_HEADERS flag on the PushPromiseFrame. This could possibly be handled in
OutgoingPushPromise though.
-
PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout
On Wed, 4 May 2022 11:05:56 GMT, Daniel Fuchs 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. > > src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java > line 881: > >> 879: nextPushStream += 2; >> 880: } >> 881: > > Hi Conor - sorry for suggesting to move this code up into > `handlePushPromise`. Now that I see the whole picture I believe these lines > should have stayed where they were, in `completePushPromise`. The main reason > is that we need to properly identify and decode possible ContinuationFrames > as being part of the push promise in order to maintain the HPack > encoders/decoders in sync, and we can only do that if we keep the > `pushContinuationState` in place until we have received the END_HEADERS flag. > > Sorry for suggesting that wrong move - it was my mistake. No problem at all Daniel. I see what you're saying, essentially that if the Continuation does come in on the wrong stream id, it still needs to be decoded and identified as a part of the Push Promise before being rejected with the wrong stream id. - PR: https://git.openjdk.java.net/jdk/pull/8518
RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout
**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. - Commit messages: - 8284585: PushPromiseContinuation test fails intermittently in timeout Changes: https://git.openjdk.java.net/jdk/pull/8518/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284585 Stats: 63 lines in 4 files changed: 43 ins; 17 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8518/head:pull/8518 PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Thu, 7 Apr 2022 13:53:35 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: Updated URI creation @Michael-Mc-Mahon @djelinski @jaikiran the src/ and test/ changes have been updated with reference to your comments. Seeking additional feedback if there is any? Also thanks for the previous reviews, all been very helpful! - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
> **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: Updated URI creation - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/92d5309a..c1ef7d29 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=03-04 Stats: 18 lines in 1 file changed: 7 ins; 0 del; 11 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
Integrated: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large
On Fri, 4 Mar 2022 14:42:40 GMT, Conor Cleary wrote: > **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. This pull request has now been integrated. Changeset: 4d2cd26a Author:Conor Cleary Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/4d2cd26ab5092ad0a169e4239164a869a4255bd3 Stats: 439 lines in 4 files changed: 397 ins; 19 del; 23 mod 8263031: HttpClient throws Exception if it receives a Push Promise that is too large Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v3]
> **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: Moved test assertions out of HttpHandlers - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/cb02c76d..132c1f31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=01-02 Stats: 33 lines in 1 file changed: 11 ins; 4 del; 18 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: 8283544: HttpClient GET method adds Content-Length: 0 header [v2]
On Wed, 30 Mar 2022 08:29:40 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: Removed testing comments On another note, I do a small check to see if the value given in the Content-length header is correct and matches up with the actual length of the body. That check is probably outside of the scope of this test and probably unecessary so I was considering removing it unless there are any arguments for keeping it. - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v2]
On Tue, 29 Mar 2022 17:09:37 GMT, Daniel Fuchs wrote: > I would not expect that throwing an AssertionError in the handler on the > server side would make the client (and the test) fail (except maybe in > timeout?). I'd suggest printing a message on System.err and sending a > different error code (like 400 for instance) if expectations are not met, and > have the client side checks that it receives 200. @dfuch Yes you're correct. I've just verified there that when there is a failure in the test's current state it is the handleResponse() method that throws an IO Exception rather than an AssertionError from the test methods. I think as you suggest, handling the Test Errors by response codes sent from the server makes the most sense in this case. - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v2]
> **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: Removed testing comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/5c1e3a72..cb02c76d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 3 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: 8283544: HttpClient GET method adds Content-Length: 0 header
On Tue, 29 Mar 2022 18:23:02 GMT, Daniel Jeliński 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.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java line
> 305:
>
>> 303: if (contentLength == 0) {
>> 304: systemHeadersBuilder.setHeader("Content-Length", "0");
>> 305: System.err.println("in 1");
>
> did you forget to remove this?
I did indeed, silly mistake on my part!
-
PR: https://git.openjdk.java.net/jdk/pull/8017
RFR: 8283544: HttpClient GET method adds Content-Length: 0 header
**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. - Commit messages: - 8283544: HttpClient GET method adds Content-Length: 0 header Changes: https://git.openjdk.java.net/jdk/pull/8017/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8017&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283544 Stats: 168 lines in 2 files changed: 159 ins; 0 del; 9 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: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v6]
On Fri, 25 Mar 2022 15:55:54 GMT, Conor Cleary wrote: >> Oh yes, good point. I think `ErrorFrame.PROTOCOL_ERROR` would be the most >> appropriate here. I'll amend the change accordingly. > > However, I'll look into the specification further for the other cases and see > if they need be changed as well. Though closing the whole connection with > `GoAwayFrame` seems correct I changed three more occurences to `ErrorFrame.PROTOCOL_ERROR`, its easier to understand and lines up more clearly with specification - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v7]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8263031: changed to ErrorFrame.PROTOCOL_ERROR - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/55e2d993..b6d34d8e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=05-06 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v6]
On Fri, 25 Mar 2022 15:51:44 GMT, Conor Cleary wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
>> line 807:
>>
>>> 805: } catch (UncheckedIOException e) {
>>> 806: debug.log("Error handling Push Promise with
>>> Continuation: " + e.getMessage(), e);
>>> 807: protocolError(ResetFrame.PROTOCOL_ERROR,
>>> e.getMessage());
>>
>> I believe it would more clear to use `ErrorFrame.PROTOCOL_ERROR` or
>> `GoAwayFrame.PROTOCOL_ERRROR` here and in the other calls to protocolError
>> below, since we're not resetting the stream here but closing the whole
>> connection with a `GoAwayFrame`.
>
> Oh yes, good point. I think `ErrorFrame.PROTOCOL_ERROR` would be the most
> appropriate here. I'll amend the change accordingly.
However, I'll look into the specification further for the other cases and see
if they need be changed as well. Though closing the whole connection with
`GoAwayFrame` seems correct
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v6]
On Thu, 24 Mar 2022 18:21:45 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8263031: further verification of push promise and response
>> - 8263031: Removed duplicate import
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 807:
>
>> 805: } catch (UncheckedIOException e) {
>> 806: debug.log("Error handling Push Promise with
>> Continuation: " + e.getMessage(), e);
>> 807: protocolError(ResetFrame.PROTOCOL_ERROR,
>> e.getMessage());
>
> I believe it would more clear to use `ErrorFrame.PROTOCOL_ERROR` or
> `GoAwayFrame.PROTOCOL_ERRROR` here and in the other calls to protocolError
> below, since we're not resetting the stream here but closing the whole
> connection with a `GoAwayFrame`.
Oh yes, good point. I think `ErrorFrame.PROTOCOL_ERROR` would be the most
appropriate here. I'll amend the change accordingly.
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. [v2]
On Tue, 15 Feb 2022 15:33:01 GMT, Conor Cleary wrote: >> As described in the title, this is a simple change to the >> `HttpRequest.Builder::build` method to highlight that an immutable and >> reusable instance of an `HttpRequest` is created when this method is >> invoked. This is done by adding an `@implSpec` Javadoc Tag (details on >> `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)). >> >> The detail added under the `@implSpec` Tag is similar to that provided at >> the interface-level Javadoc for Builder. >> >> Please also review the CSR for this change: >> https://bugs.openjdk.java.net/browse/JDK-8281833 > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8281223: Updated description, added missing code tag Awaiting CSR approval and additonal review, keeping this PR active by commenting - PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v5]
On Tue, 22 Mar 2022 10:57:35 GMT, Daniel Fuchs wrote: >> Conor Cleary has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - 8263031: Added return statements and pcs var >> - 8263031: Test cleanup, added third case >> - Revert "Test cleanup, added third case" >> >>This reverts commit 5c1c0f5fea801923bc3352bf88c701768d78b11d. >> - Test cleanup, added third case > > test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 158: > >> 156: CompletableFuture> cf = >> 157: client.sendAsync(hreq, >> HttpResponse.BodyHandlers.ofString(UTF_8), pph); >> 158: cf.join(); > > I'd suggest checking that we received the expected response code + body too Further changes made to check response codes and bodies of both the response and push promise. - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v6]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with two additional commits since the last revision: - 8263031: further verification of push promise and response - 8263031: Removed duplicate import - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/c0543da9..55e2d993 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=04-05 Stats: 48 lines in 2 files changed: 15 ins; 16 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v5]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with four additional commits since the last revision: - 8263031: Added return statements and pcs var - 8263031: Test cleanup, added third case - Revert "Test cleanup, added third case" This reverts commit 5c1c0f5fea801923bc3352bf88c701768d78b11d. - Test cleanup, added third case - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/c2ae1e56..c0543da9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=03-04 Stats: 27 lines in 2 files changed: 18 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v4]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8263031: Correct ordering of imports - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/c14f599e..c2ae1e56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=02-03 Stats: 55 lines in 1 file changed: 27 ins; 28 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:08:49 GMT, Daniel Fuchs wrote: >> Conor Cleary has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8263031: Tidied up import statements > > src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java > line 82: > >> 80: import java.util.function.Function; >> 81: import java.util.function.Supplier; >> 82: > > Could we keep the original alphabetical ordering and have the java.* imports > come before the jdk.* imports? Done, this was just an IDE hiccup - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v3]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with two additional commits since the last revision: - 8263031: Cleanup of changes in Http2Connection - 8263031: Added test for multiple Continuation Frames - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/664a1105..c14f599e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=01-02 Stats: 117 lines in 2 files changed: 74 ins; 11 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:31:41 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8263031: Tidied up import statements
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 855:
>
>> 853:
>> 854: private record PushContinuationState(HeaderDecoder pushContDecoder,
>> PushPromiseFrame pushContFrame) {}
>> 855: private volatile PushContinuationState pcs;
>
> I'd prefer to have a longer name than `pcs` - `pushContinuationState` would
> make it less mysterious at the place where it's used. Also it could be good
> to move these declarations (linew 854 and 855) higher in the file, where
> other instance variables are declared.
Changed to pushContinuationState, its a long name but definitely clearer
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 14 Mar 2022 11:19:36 GMT, Conor Cleary wrote:
>> test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 176:
>>
>>> 174: ContinuationFrame cf = new ContinuationFrame(streamid,
>>> HeaderFrame.END_HEADERS, encodedHeaders);
>>> 175:
>>> 176: try {
>>
>> It would be good to have a test-case that creates 2 continuation frames too.
>
> Good idea yes, to check that the repeat continuation still behaves as
> expected. Should hopefully be straight forward to create another test case.
On this issue, there is a case where a faulty server might send an indefinite
number of Continuations (maybe the server never sets an END_HEADERS flag).
Should a safe guard for the Push Promise with Continuation/s case be put in
place to prevent the faulty scenario?
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:17:53 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8263031: Tidied up import statements
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 803:
>
>> 801: if (pcs != null) {
>> 802: if (frame instanceof ContinuationFrame cf) {
>> 803: handlePushContinuation(stream, cf);
>
> IOException should probably be caught and transformed in protocol error here
> to?
> In case of protocol error when handling push promise (or their continuation)
> - should `pcs` be reset to null?
> Maybe it doesn't matter since we're closing the connection anyway.
In my opinion it would probably be safest to set pcs to null here yes.
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:28:29 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8263031: Tidied up import statements
>
> test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 176:
>
>> 174: ContinuationFrame cf = new ContinuationFrame(streamid,
>> HeaderFrame.END_HEADERS, encodedHeaders);
>> 175:
>> 176: try {
>
> It would be good to have a test-case that creates 2 continuation frames too.
Good idea yes, to check that the repeat continuation still behaves as expected.
Should hopefully be straight forward to create another test case.
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 08:23:46 GMT, Conor Cleary wrote:
>> **Problem**
>> When a Continuation Frame is received by the httpclient using HTTP/2 after a
>> Push Promise frame (can happen if the amount of headers to be sent in a
>> single Push Promise frame exceeds the maximum frame size, so a Continuation
>> frame is required), the following exception occurs:
>>
>>
>> java.io.IOException: no statuscode in response
>> at
>> java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
>> at
>> java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
>> ...
>>
>> This exception occurs because there is no existing flow in
>> `jdk/internal/net/http/Http2Connection.java` which accounts for the case
>> where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0.
>> When this occurs, the only acceptable frame/s (as multiple continuations are
>> also acceptable) that can be received by the client on the same stream is a
>> continuation frame.
>>
>> **Fix**
>> To ensure correct behavior, the following changes were made to
>> `jdk/internal/net/http/Http2Connection.java`.
>>
>> - The existing method `handlePushPromise()` was modified so that if the
>> END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to
>> track the state of the Push Promise containing a shared `HeaderDecoder` and
>> the received `PushPromiseFrame` is initialised.
>> - When the subsequent `ContinuationFrame` is received in `processFrame()`,
>> the method `handlePushContinuation()` is called instead of the default flow
>> resulting in `stream.incoming(frame)` being called (the source of the
>> incorrect behaviour originally).
>> - In `handlePushContinuation()`, the shared decoder is used to decode the
>> received `ContinuationFrame` headers and if the `END_HEADERS` flag is set
>> (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a
>> whole is constructed which serves to combine the headers from both the
>> `PushPromiseFrame` and the `ContinuationFrame`.
>>
>> A regression test was included which verifies that the exception is not
>> thrown and that the headers arrive correctly.
>
> Conor Cleary has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8263031: Tidied up import statements
test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 227:
> 225: map.put("x-promise", List.of(mainPromiseBody));
> 226: HttpHeaders headers = HttpHeaders.of(map, ACCEPT_ALL);
> 227: exchange.serverPush(uri, headers, is);
Could create all of the headers here instead of in Http2LPPTestExchangeImpl,
might improve readability of test.
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large
On Fri, 4 Mar 2022 15:17:11 GMT, Conor Cleary wrote: > I see that a couple of imports got changed by my IDE, will address that > shortly Now resolved - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
> **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8263031: Tidied up import statements - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/527cfacb..664a1105 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=00-01 Stats: 35 lines in 1 file changed: 31 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large
On Fri, 4 Mar 2022 14:42:40 GMT, Conor Cleary wrote: > **Problem** > When a Continuation Frame is received by the httpclient using HTTP/2 after a > Push Promise frame (can happen if the amount of headers to be sent in a > single Push Promise frame exceeds the maximum frame size, so a Continuation > frame is required), the following exception occurs: > > > java.io.IOException: no statuscode in response > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > ... > > This exception occurs because there is no existing flow in > `jdk/internal/net/http/Http2Connection.java` which accounts for the case > where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. > When this occurs, the only acceptable frame/s (as multiple continuations are > also acceptable) that can be received by the client on the same stream is a > continuation frame. > > **Fix** > To ensure correct behavior, the following changes were made to > `jdk/internal/net/http/Http2Connection.java`. > > - The existing method `handlePushPromise()` was modified so that if the > END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track > the state of the Push Promise containing a shared `HeaderDecoder` and the > received `PushPromiseFrame` is initialised. > - When the subsequent `ContinuationFrame` is received in `processFrame()`, > the method `handlePushContinuation()` is called instead of the default flow > resulting in `stream.incoming(frame)` being called (the source of the > incorrect behaviour originally). > - In `handlePushContinuation()`, the shared decoder is used to decode the > received `ContinuationFrame` headers and if the `END_HEADERS` flag is set > (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a > whole is constructed which serves to combine the headers from both the > `PushPromiseFrame` and the `ContinuationFrame`. > > A regression test was included which verifies that the exception is not > thrown and that the headers arrive correctly. I see that a couple of imports got changed by my IDE, will address that shortly - PR: https://git.openjdk.java.net/jdk/pull/7696
RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large
**Problem** When a Continuation Frame is received by the httpclient using HTTP/2 after a Push Promise frame (can happen if the amount of headers to be sent in a single Push Promise frame exceeds the maximum frame size, so a Continuation frame is required), the following exception occurs: java.io.IOException: no statuscode in response at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) ... This exception occurs because there is no existing flow in `jdk/internal/net/http/Http2Connection.java` which accounts for the case where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. When this occurs, the only acceptable frame/s (as multiple continuations are also acceptable) that can be received by the client on the same stream is a continuation frame. **Fix** To ensure correct behavior, the following changes were made to `jdk/internal/net/http/Http2Connection.java`. - The existing method `handlePushPromise()` was modified so that if the END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track the state of the Push Promise containing a shared `HeaderDecoder` and the received `PushPromiseFrame` is initialised. - When the subsequent `ContinuationFrame` is received in `processFrame()`, the method `handlePushContinuation()` is called instead of the default flow resulting in `stream.incoming(frame)` being called (the source of the incorrect behaviour originally). - In `handlePushContinuation()`, the shared decoder is used to decode the received `ContinuationFrame` headers and if the `END_HEADERS` flag is set (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a whole is constructed which serves to combine the headers from both the `PushPromiseFrame` and the `ContinuationFrame`. - Commit messages: - 8263031: Created new flow for Push Promises followed by Continuations - 8263031: Setting END_HEADERS flag where appropriate - 8263031: Update test name and cleanup - 8263031: HttpClient throws Exception if it receives a Push Promise that is too large Changes: https://git.openjdk.java.net/jdk/pull/7696/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263031 Stats: 423 lines in 4 files changed: 336 ins; 67 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/7696.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696 PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. [v2]
> As described in the title, this is a simple change to the > `HttpRequest.Builder::build` method to highlight that an immutable and > reusable instance of an `HttpRequest` is created when this method is invoked. > This is done by adding an `@implSpec` Javadoc Tag (details on `@implSpec` > given [here](https://openjdk.java.net/jeps/8068562)). > > The detail added under the `@implSpec` Tag is similar to that provided at the > interface-level Javadoc for Builder. > > Please also review the CSR for this change: > https://bugs.openjdk.java.net/browse/JDK-8281833 Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8281223: Updated description, added missing code tag - Changes: - all: https://git.openjdk.java.net/jdk/pull/7479/files - new: https://git.openjdk.java.net/jdk/pull/7479/files/1bead0bd..bfc1de70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7479/head:pull/7479 PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects.
On Tue, 15 Feb 2022 15:04:12 GMT, Daniel Fuchs wrote:
>> As described in the title, this is a simple change to the
>> `HttpRequest.Builder::build` method to highlight that an immutable and
>> reusable instance of an `HttpRequest` is created when this method is
>> invoked. This is done by adding an `@implSpec` Javadoc Tag (details on
>> `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)).
>>
>> The detail added under the `@implSpec` Tag is similar to that provided at
>> the interface-level Javadoc for Builder.
>>
>> Please also review the CSR for this change:
>> https://bugs.openjdk.java.net/browse/JDK-8281833
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 297:
>
>> 295: * @implSpec Returns a new {@code HttpRequest} each time it is
>> 296: * invoked. Once built, the HttpRequest is immutable and can be
>> 297: * sent multiple times.
>
> Looks fine to me. Maybe "This method returns ..." would more appropriate for
> an `@implSpec`. Also the second mention of `HttpRequest` should also have
> `{@code }` around it.
Agree with both statements. "This method returns..." adds additional clarity
and the additional {@code } was left out in error. Will correct now.
-
PR: https://git.openjdk.java.net/jdk/pull/7479
RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects.
As described in the title, this is a simple change to the `HttpRequest.Builder::build` method to highlight that an immutable and reusable instance of an `HttpRequest` is created when this method is invoked. This is done by adding an `@implSpec` Javadoc Tag (details on `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)). The detail added under the `@implSpec` Tag is similar to that provided at the interface-level Javadoc for Builder. Please also review the CSR for this change: https://bugs.openjdk.java.net/browse/JDK-8281833 - Commit messages: - 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. Changes: https://git.openjdk.java.net/jdk/pull/7479/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281223 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7479/head:pull/7479 PR: https://git.openjdk.java.net/jdk/pull/7479
Integrated: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly
On Mon, 12 Apr 2021 15:21:05 GMT, Conor Cleary wrote: > ### Description > `Inet6Address/B6206527.java` test creates two instances of ServerSocket, both > of which are explicity bound to a Link-Local address. Neither of the > ServerSocket instances are explicitly closed meaning there is no guarantee > that their associated resources are freed. > > ### Fix > Each ServerSocket is instantiated in a try-with-resources block. This ensures > that in both cases of success or failure within the try-with-resources block, > the sockets are always closed thanks to ServerSocket implementing Closeable. > The test is also now started in othervm mode as an added assurance of the > test's isolation in the event that resources are not freed. This pull request has now been integrated. Changeset: 6293299d Author:Conor Cleary Committer: Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/6293299d Stats: 11 lines in 1 file changed: 2 ins; 0 del; 9 mod 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly Reviewed-by: aefimov, dfuchs, michaelm, vtewari - PR: https://git.openjdk.java.net/jdk/pull/3437
Re: RFR: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly [v2]
On Tue, 13 Apr 2021 09:04:02 GMT, Michael McMahon wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed othervm argument
>
> test/jdk/java/net/Inet6Address/B6206527.java line 47:
>
>> 45: }
>> 46:
>> 47: try (ServerSocket ss = new ServerSocket()) {
>
> Maybe for brevity you could use `try (var ss = new ServerSocket()) {`
>
> Only a suggestion. It's up to you.
Though I agree that the brevity is nice, I think I will leave it as is
(`ServerSocket` instead of `var`) in the interest of clarity on the type of
instance being created in both test cases as it was the original version of the
test.
-
PR: https://git.openjdk.java.net/jdk/pull/3437
Re: RFR: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly [v2]
> ### Description > `Inet6Address/B6206527.java` test creates two instances of ServerSocket, both > of which are explicity bound to a Link-Local address. Neither of the > ServerSocket instances are explicitly closed meaning there is no guarantee > that their associated resources are freed. > > ### Fix > Each ServerSocket is instantiated in a try-with-resources block. This ensures > that in both cases of success or failure within the try-with-resources block, > the sockets are always closed thanks to ServerSocket implementing Closeable. > The test is also now started in othervm mode as an added assurance of the > test's isolation in the event that resources are not freed. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: Removed othervm argument - Changes: - all: https://git.openjdk.java.net/jdk/pull/3437/files - new: https://git.openjdk.java.net/jdk/pull/3437/files/2cbd69a6..42312d1b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3437&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3437&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3437/head:pull/3437 PR: https://git.openjdk.java.net/jdk/pull/3437
Re: RFR: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly
On Tue, 13 Apr 2021 09:14:47 GMT, Daniel Fuchs wrote: >> test/jdk/java/net/Inet6Address/B6206527.java line 31: >> >>> 29: * @build jdk.test.lib.NetworkConfiguration >>> 30: *jdk.test.lib.Platform >>> 31: * @run main/othervm B6206527 >> >> Does it need othervm mode? Best to not use it if it can be avoided. > > I agree with Michael that the test doesn't need the `/othervm` mode. I included it originally as the issue was related to sockets not being closed and running in `othervm` mitigates negative effects associated with that. However using the try-with-resources prevents this from being the case, so I agree that it is no longer required and will update accordingly. - PR: https://git.openjdk.java.net/jdk/pull/3437
RFR: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly
### Description `Inet6Address/B6206527.java` test creates two instances of ServerSocket, both of which are explicity bound to a Link-Local address. Neither of the ServerSocket instances are explicitly closed meaning there is no guarantee that their associated resources are freed. ### Fix Each ServerSocket is instantiated in a try-with-resources block. This ensures that in both cases of success or failure within the try-with-resources block, the sockets are always closed thanks to ServerSocket implementing Closeable. The test is also now started in othervm mode as an added assurance of the test's isolation in the event that resources are not freed. - Commit messages: - 8264824: Removed erroneous whitespace - 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly Changes: https://git.openjdk.java.net/jdk/pull/3437/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3437&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264824 Stats: 12 lines in 1 file changed: 2 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3437/head:pull/3437 PR: https://git.openjdk.java.net/jdk/pull/3437
Integrated: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
On Wed, 24 Feb 2021 10:24:06 GMT, Conor Cleary wrote:
> A number of net tests use a
> **[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
> to verify either the functionality of this type of Name Service or as a
> complement to other tests (such as Caching, Address Format etc.).
>
> Intermittent failures of these tests can be caused by tools used during JVM
> start-up accessing/initialising classes sooner than may be expected which can
> cause unexpected behaviour. Most commonly, this unexpected behaviour takes
> the form of the
> **[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
> being used despite a call to `System.setProperty("jdk.net.hosts.file", ...)`
> in the test file. Due to the fact that **InetAddress** initialises it's
> Implementation and Name Service fields in a static class initialiser ([see
> L1132 of
> InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
> and that the default mode is to use the **Platform Name Service**, setting a
> system property in the main method to specify the use of
> **HostsFileNameService** (with the jdk.net.hosts.file property)
has no affect if the class has been previously accessed during JVM start-up
and **InetAddress** will default to the **PlatformNameService** which is
unsuitable for this test. This explains the intermittent failures caused by the
use of `System.setProperty("jdk.net.hosts.file", ...)`.
>
> This fix improves the robustness of this test by specifying the use of the
> **HostsFileNameService** in the `@run` tag for this test via the previously
> mentioned system property. This gives certainty that the property will be
> properly set in time for the actual run of this test after the JVM has
> booted. An example of one the fixes...
> * @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
> textToNumericFormat
This pull request has now been integrated.
Changeset: 8bc8542e
Author:Conor Cleary
Committer: Aleksei Efimov
URL: https://git.openjdk.java.net/jdk/commit/8bc8542e
Stats: 72 lines in 11 files changed: 8 ins; 42 del; 22 mod
8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file
property)
Reviewed-by: michaelm, aefimov, dfuchs, chegar
-
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: 8253100: Fix "no comment" warnings in java.base/java.net [v2]
On Fri, 26 Feb 2021 10:44:04 GMT, Rahul Yadav wrote: >> Daniel Fuchs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed comment > > LGTM! Ran the changes through `-Xdoclint:missing` and there were no warnings outputted so looks good! - PR: https://git.openjdk.java.net/jdk/pull/2715
Re: RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property) [v2]
On Thu, 25 Feb 2021 17:26:00 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8262195: Copyrights & InternalNSTest Host File Delete
>> - 8262195: Added descriptive comments and filename
>
> test/jdk/java/net/InetAddress/InternalNameServiceWithNoHostsFileTest.java
> line 32:
>
>> 30: * thrown
>> 31: * @run main/othervm -Djdk.net.hosts.file=TestHosts-II
>> -Dsun.net.inetaddr.ttl=0
>> 32: * InternalNameServiceWithNoHostsFileTest
>
> shouldn't this be:
>
> @run main/othervm -Djdk.net.hosts.file=${test.src}/TestHosts-II
> -Dsun.net.inetaddr.ttl=0
TestHosts-II was never in the test.src directory, from what I could tell it was
just a dummy file (as per the test name) to verify the reaction when a Hosts
File is not found. So, to be consistent with the other changes in this PR where
a Host File is not present, I gave just the Filename in the path. Are there
possible issues stemming from this? One I can think of is that a Hosts FIle of
that name happens to be present in the user.dir though that is probably not
very likely.
-
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property) [v2]
> A number of net tests use a
> **[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
> to verify either the functionality of this type of Name Service or as a
> complement to other tests (such as Caching, Address Format etc.).
>
> Intermittent failures of these tests can be caused by tools used during JVM
> start-up accessing/initialising classes sooner than may be expected which can
> cause unexpected behaviour. Most commonly, this unexpected behaviour takes
> the form of the
> **[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
> being used despite a call to `System.setProperty("jdk.net.hosts.file", ...)`
> in the test file. Due to the fact that **InetAddress** initialises it's
> Implementation and Name Service fields in a static class initialiser ([see
> L1132 of
> InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
> and that the default mode is to use the **Platform Name Service**, setting a
> system property in the main method to specify the use of
> **HostsFileNameService** (with the jdk.net.hosts.file property)
has no affect if the class has been previously accessed during JVM start-up
and **InetAddress** will default to the **PlatformNameService** which is
unsuitable for this test. This explains the intermittent failures caused by the
use of `System.setProperty("jdk.net.hosts.file", ...)`.
>
> This fix improves the robustness of this test by specifying the use of the
> **HostsFileNameService** in the `@run` tag for this test via the previously
> mentioned system property. This gives certainty that the property will be
> properly set in time for the actual run of this test after the JVM has
> booted. An example of one the fixes...
> * @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
> textToNumericFormat
Conor Cleary has updated the pull request incrementally with two additional
commits since the last revision:
- 8262195: Copyrights & InternalNSTest Host File Delete
- 8262195: Added descriptive comments and filename
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2703/files
- new: https://git.openjdk.java.net/jdk/pull/2703/files/84a0cb2c..f7e0464f
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2703&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2703&range=00-01
Stats: 16 lines in 8 files changed: 2 ins; 4 del; 10 mod
Patch: https://git.openjdk.java.net/jdk/pull/2703.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2703/head:pull/2703
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
On Wed, 24 Feb 2021 16:10:01 GMT, Aleksei Efimov wrote:
>> A number of net tests use a
>> **[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
>> to verify either the functionality of this type of Name Service or as a
>> complement to other tests (such as Caching, Address Format etc.).
>>
>> Intermittent failures of these tests can be caused by tools used during JVM
>> start-up accessing/initialising classes sooner than may be expected which
>> can cause unexpected behaviour. Most commonly, this unexpected behaviour
>> takes the form of the
>> **[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
>> being used despite a call to `System.setProperty("jdk.net.hosts.file",
>> ...)` in the test file. Due to the fact that **InetAddress** initialises
>> it's Implementation and Name Service fields in a static class initialiser
>> ([see L1132 of
>> InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
>> and that the default mode is to use the **Platform Name Service**, setting
>> a system property in the main method to specify the use of
>> **HostsFileNameService** (with the jdk.net.hosts.file property)
has no affect if the class has been previously accessed during JVM start-up
and **InetAddress** will default to the **PlatformNameService** which is
unsuitable for this test. This explains the intermittent failures caused by the
use of `System.setProperty("jdk.net.hosts.file", ...)`.
>>
>> This fix improves the robustness of this test by specifying the use of the
>> **HostsFileNameService** in the `@run` tag for this test via the previously
>> mentioned system property. This gives certainty that the property will be
>> properly set in time for the actual run of this test after the JVM has
>> booted. An example of one the fixes...
>> * @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
>> textToNumericFormat
>
> Thanks for cleaning-up tests, Conor.
> NIT: You also update/add the modification year in test headers.
> If a tool (used during JVM start-up) does more than just initialize the class
> (maybe it initiates a lookup), then it will find that it is out of luck - the
> hosts file specified by the property on the command line may not exist. Such
> a tool would get an UHE with a message similar to: "Unable to resolve host
> foo as hosts file not found". Would this be an issue for such a tool?
It would depend on how the tool handles such an exception. In my opinion if the
HostsFileNameService is being used to resolve addresses in testing the onus
falls on the developer to ensure that such a tool can still function correctly
in this situation. For example, a tool might silently catch the exception
thrown and still function as intended. Or if it did the opposite, a different
approach may be needed.
-
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
On Wed, 24 Feb 2021 15:57:49 GMT, Aleksei Efimov wrote:
>> A number of net tests use a
>> **[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
>> to verify either the functionality of this type of Name Service or as a
>> complement to other tests (such as Caching, Address Format etc.).
>>
>> Intermittent failures of these tests can be caused by tools used during JVM
>> start-up accessing/initialising classes sooner than may be expected which
>> can cause unexpected behaviour. Most commonly, this unexpected behaviour
>> takes the form of the
>> **[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
>> being used despite a call to `System.setProperty("jdk.net.hosts.file",
>> ...)` in the test file. Due to the fact that **InetAddress** initialises
>> it's Implementation and Name Service fields in a static class initialiser
>> ([see L1132 of
>> InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
>> and that the default mode is to use the **Platform Name Service**, setting
>> a system property in the main method to specify the use of
>> **HostsFileNameService** (with the jdk.net.hosts.file property)
has no affect if the class has been previously accessed during JVM start-up
and **InetAddress** will default to the **PlatformNameService** which is
unsuitable for this test. This explains the intermittent failures caused by the
use of `System.setProperty("jdk.net.hosts.file", ...)`.
>>
>> This fix improves the robustness of this test by specifying the use of the
>> **HostsFileNameService** in the `@run` tag for this test via the previously
>> mentioned system property. This gives certainty that the property will be
>> properly set in time for the actual run of this test after the JVM has
>> booted. An example of one the fixes...
>> * @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
>> textToNumericFormat
>
> test/jdk/java/net/InetAddress/InternalNameServiceTest.java line 30:
>
>> 28: * a file name that contains address host mappings, similar to
>> those in
>> 29: * /etc/hosts file.
>> 30: * @run main/othervm -Djdk.net.hosts.file=${test.src}/TestHosts
>> -Dsun.net.inetaddr.ttl=0
>
> Maybe the `TestHosts` file can be removed from the repository, and
> `-Djdk.net.hosts.file` value can be changed to `TestHosts` file:
> With such change the test host file will be generated in `scratch` directory,
> and in case of test failures it won't modify the file stored in repository.
I was thinking this also, will update that in my next push along with the
copyright years (oops!)
-
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
On Wed, 24 Feb 2021 10:24:06 GMT, Conor Cleary wrote:
> A number of net tests use a
> **[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
> to verify either the functionality of this type of Name Service or as a
> complement to other tests (such as Caching, Address Format etc.).
>
> Intermittent failures of these tests can be caused by tools used during JVM
> start-up accessing/initialising classes sooner than may be expected which can
> cause unexpected behaviour. Most commonly, this unexpected behaviour takes
> the form of the
> **[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
> being used despite a call to `System.setProperty("jdk.net.hosts.file", ...)`
> in the test file. Due to the fact that **InetAddress** initialises it's
> Implementation and Name Service fields in a static class initialiser ([see
> L1132 of
> InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
> and that the default mode is to use the **Platform Name Service**, setting a
> system property in the main method to specify the use of
> **HostsFileNameService** (with the jdk.net.hosts.file property)
has no affect if the class has been previously accessed during JVM start-up
and **InetAddress** will default to the **PlatformNameService** which is
unsuitable for this test. This explains the intermittent failures caused by the
use of `System.setProperty("jdk.net.hosts.file", ...)`.
>
> This fix improves the robustness of this test by specifying the use of the
> **HostsFileNameService** in the `@run` tag for this test via the previously
> mentioned system property. This gives certainty that the property will be
> properly set in time for the actual run of this test after the JVM has
> booted. An example of one the fixes...
> * @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
> textToNumericFormat
The following host files have been removed,
`test/jdk/java/net/Inet4Address/TestToNumericFormatHosts`,
`test/jdk/sun/net/InetAddress/nameservice/simple/DefaultCachingHosts` and
`test/jdk/sun/net/InetAddress/nameservice/simple/CacheTestHosts`.
These host files are concerned with
`test/jdk/java/net/Inet4Address/textToNumericFormat.java`,
`test/jdk/sun/net/InetAddress/nameservice/simple/DefaultCaching.java` and
`test/jdk/sun/net/InetAddress/nameservice/simple/CacheTest.java` respectively.
In the case of the latter two tests in
`sun/net/InetAddress/nameservice/simple/`, the hosts file is created from the
use of an `addMappingToHostsFile(...)` method which subsequently creates a new
FileWriter object. If the file does not exist, the FileWriter will create it
based off of the "jdk.net.hosts.file" property which in the case of these tests
will write to the current directory ("user.dir", the paths are relative so
`-Djdk.net.hosts.file=some/hosts/file` is valid). If the file does exist, it
will write to it as needed by the test. Given that the hosts file is created as
needed by the FileWriter, there is less need for either
`simple/DefaultCachingHosts` and `simple/CacheTest` to be present.
For `test/jdk/java/net/Inet4Address/textToNumericFormat.java`, there is a
little further explanation required. This test is concerned with testing if
InetAddress correctly identifies well formatted and badly formatted IP address
literals. For example "224.0.1.0" (good) vs. "238.255.255.2550" (bad). The test
makes use of `InetAddress.getByName()`which will not perform a lookup if given
an IPv4 address. For "good" addresses, a new InetAddress with a null hostName
is returned (which is discarded). For "bad" addresses an UnknownHostException
is thrown. This behaviour can be seen at InetAddress
If its the case that a PlatformNameService is unexpectedly used, the test
behaviour is mostly the same (although there is a chance that the addresses
will not be found). However the test will take far longer to complete as the
PlatformNameService will attempt to resolve the "good" addresses concerned
rather than just cross-check a hosts file. Of course, the original hosts file
is empty as we are not actually looking to resolve names, just that InetAddress
can detect a textual representation of an IP address. Therefore a hosts file is
not needed here, all that is needed is for the HostsFileNameService to be used
for name lookups.
-
PR: https://git.openjdk.java.net/jdk/pull/2703
RFR: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
A number of net tests use a
**[HostsFileNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L955)**
to verify either the functionality of this type of Name Service or as a
complement to other tests (such as Caching, Address Format etc.).
Intermittent failures of these tests can be caused by tools used during JVM
start-up accessing/initialising classes sooner than may be expected which can
cause unexpected behaviour. Most commonly, this unexpected behaviour takes the
form of the
**[PlatformNameService](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L927)**
being used despite a call to `System.setProperty("jdk.net.hosts.file", ...)`
in the test file. Due to the fact that **InetAddress** initialises it's
Implementation and Name Service fields in a static class initialiser ([see
L1132 of
InetAddress.java](https://github.com/openjdk/jdk/blob/382e38dd246596ec94a1f1ce0e0f9e87f53366c7/src/java.base/share/classes/java/net/InetAddress.java#L1132))
and that the default mode is to use the **Platform Name Service**, setting a
system property in the main method to specify the use of
**HostsFileNameService** (with the jdk.net.hosts.file property) ha
s no affect if the class has been previously accessed during JVM start-up and
**InetAddress** will default to the **PlatformNameService** which is unsuitable
for this test. This explains the intermittent failures caused by the use of
`System.setProperty("jdk.net.hosts.file", ...)`.
This fix improves the robustness of this test by specifying the use of the
**HostsFileNameService** in the `@run` tag for this test via the previously
mentioned system property. This gives certainty that the property will be
properly set in time for the actual run of this test after the JVM has booted.
An example of one the fixes...
* @run main/othervm -Djdk.net.hosts.file=TestToNumericFormatHosts
textToNumericFormat
-
Commit messages:
- 8262195: Removed Hostfiles
- 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file
property)
Changes: https://git.openjdk.java.net/jdk/pull/2703/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2703&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8262195
Stats: 59 lines in 10 files changed: 6 ins; 38 del; 15 mod
Patch: https://git.openjdk.java.net/jdk/pull/2703.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2703/head:pull/2703
PR: https://git.openjdk.java.net/jdk/pull/2703
Re: RFR: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK [v2]
On Thu, 18 Feb 2021 10:12:30 GMT, Conor Cleary wrote: >>> I think that the changes are mostly good. I would like to try them out on >>> my local system and our internal buildAndTest system. >> >> Chris, were you able to test the patch? >> >> Also if anyone has an idea as to the best way to unit test this, let me >> know; are there any other unit tests where the build can become a root user? > >> > I think that the changes are mostly good. I would like to try them out on >> > my local system and our internal buildAndTest system. >> >> Chris, were you able to test the patch? >> >> Also if anyone has an idea as to the best way to unit test this, let me >> know; are there any other unit tests where the build can become a root user? > > Hi @jamieletual, I'm currently investigating any possibilities in testing for > this change. In the meantime however it could be good to change the title of > the PR to "8257235: InetAddress.isReachable should use non-privileged ICMP > sockets when available" as it appears to be an Integration blocker (see > Integration Blocker section of first PR comment). Test failures related to these change are seen in the following areas after tier 1-3 test runs and runs across net & nio packages: **test/jdk/java/net/Inet4Address/PingThis.java (macosx-x64, linux-x64)** --System.out:(1/34)-- localhost/127.0.0.1 is reachable --System.err:(12/791)-- java.lang.RuntimeException: Unexpected exception:java.net.SocketException: Can't assign requested address at IsReachableViaLoopbackTest.main(IsReachableViaLoopbackTest.java:36) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298) at java.base/java.lang.Thread.run(Thread.java:831) **test/jdk/java/net/InetAddress/IsReachableViaLoopbackTest.java (macosx-x64):** --System.out:(1/25)-- The target ip is 0.0.0.0 --System.err:(16/981)-- java.net.SocketException: Socket is not connected at java.base/java.net.Inet6AddressImpl.isReachable0(Native Method) at java.base/java.net.Inet6AddressImpl.isReachable(Inet6AddressImpl.java:94) at java.base/java.net.InetAddress.isReachable(InetAddress.java:548) at java.base/java.net.InetAddress.isReachable(InetAddress.java:507) at PingThis.main(PingThis.java:66) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298) at java.base/java.lang.Thread.run(Thread.java:831) Both tests call the native isReachable0 and it seems related to the version in Inet6AddressImpl.c - PR: https://git.openjdk.java.net/jdk/pull/1502
Re: RFR: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK [v2]
On Fri, 19 Feb 2021 14:48:39 GMT, Conor Cleary wrote: >> Jamie Le Tual has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed formatting > > src/java.base/unix/native/libnet/Inet4AddressImpl.c line 375: > >> 373: icmp->icmp_type = ICMP_ECHO; >> 374: icmp->icmp_code = 0; >> 375: // same result as downcasting the little-endian pid, although >> we are not longer > > I don't think this is true. When downcasting the pid (which is at this stage > in Nework/Big-Endian Order), the host order will be considered. Assuming that > the downcast is to `uint16_t`, which it looks like is what icmp_id takes, the > 16 least significant bits will be considered (if the host machine is > little-endian of course). This is different from the 16 bit right shift. Here > is some example output from a short C program I wrote (output is in hex to > more easily demonstrate bitwise ops): > > pid size: 4 > int size: 4 > > getpid() testing: > pid = be67f > htonl = 7fe60b00 > cast = b00 >>> 16 = 7fe6 On that note, is it ok to ignore any bits that are shifted out? For example, `7fe60b00 >> 16` results in `7fe6` while the trailing `0b00` is ignored. - PR: https://git.openjdk.java.net/jdk/pull/1502
Re: RFR: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK [v2]
On Wed, 23 Dec 2020 14:06:12 GMT, Jamie Le Tual wrote: >> Users have been able to send ICMP packets without the need for root >> privileges or the CAP_NET_RAW capability since at least kernel 3.11. >> >> For some time now, if the kernel parameter net.ipv4.ping_group_range >> included the gid of a user sending an icmp packet with the IPPROTO_ICMP >> protocol, then the packet would> >> It's important to note that the both the checksum and ident field are >> overwritten by the kernel when this is done. >> >> Newer distributions are now setting the default value of >> net.ipv4.ping_group_range to be open to all possible group ids (Fedora 31 >> and Ubuntu 20.04 for example) so it can b> >> >> Also of note is the that this is also implemented in MacOS. >> >> This patch proposes attempting to use IPPROTO_ICMP first, and then fall back >> to attempting a raw socket and ultimately failing over to tcp echo. >> This patch also alters the logic for identifying icmp reply packets, since >> the kernel overwrites id ident field when using the IPPROTO_ICMP protocol. >> The method is similar to that used by the ping(8) utility in the iputils >> package, where we compare data in the icmp_data member of the icmp struct >> to identify the packet as our response. The ping utility compares the >> timeval, whereas this patch proposes to compare both the timeval and the >> user's pid. > > Jamie Le Tual has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed formatting src/java.base/unix/native/libnet/Inet4AddressImpl.c line 375: > 373: icmp->icmp_type = ICMP_ECHO; > 374: icmp->icmp_code = 0; > 375: // same result as downcasting the little-endian pid, although we > are not longer I don't think this is true. When downcasting the pid (which is at this stage in Nework/Big-Endian Order), the host order will be considered. Assuming that the downcast is to `uint16_t`, which it looks like is what icmp_id takes, the 16 least significant bits will be considered (if the host machine is little-endian of course). This is different from the 16 bit right shift. Here is some example output from a short C program I wrote (output is in hex to more easily demonstrate bitwise ops): pid size: 4 int size: 4 getpid() testing: pid = be67f htonl = 7fe60b00 cast = b00 >> 16 = 7fe6 - PR: https://git.openjdk.java.net/jdk/pull/1502
Re: RFR: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK [v2]
On Tue, 16 Feb 2021 13:37:20 GMT, Jamie Le Tual wrote: > > I think that the changes are mostly good. I would like to try them out on > > my local system and our internal buildAndTest system. > > Chris, were you able to test the patch? > > Also if anyone has an idea as to the best way to unit test this, let me know; > are there any other unit tests where the build can become a root user? Hi @jamieletual, I'm currently investigating any possibilities in testing for this change. In the meantime however it could be good to change the title of the PR to "8257235: InetAddress.isReachable should use non-privileged ICMP sockets when available" as it appears to be an Integration blocker (see Integration Blocker section of first PR comment). - PR: https://git.openjdk.java.net/jdk/pull/1502
Re: RFR: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK [v2]
On Tue, 16 Feb 2021 13:31:13 GMT, Jamie Le Tual wrote: >> src/java.base/unix/native/libnet/Inet6AddressImpl.c line 713: >> >>> 711: This usually requires "root" privileges, so it's likely to fail. >>> 712: If all else fails, fall back to TCP and implement tcp echo >>> 713: */ >> >> This is one of the block comments that needs a tidy, same thing in >> Inet4Address.c. Also check the // comments and you'll see some of the >> inconsistencies there. It's just nit picking, the patch itself is good, just >> hard to test. > > OK, is it the fact that it's a block comment instead of a line comment? Is > the indentation not right? > I don't see what needs to be cleaned up I think what Alan said was in reference to the overall styling of the block comment. For example, something like this with more consistent spacing may be more appropriate (note the minimal spacing at the end of sentences where possible). Perhaps the block comment could be indented also to match the block of code its concerning. /* First we see if the OS supports ICMP as a protocol. In the event that this is an older kernel without IPPROTO_ICMP or that the net.ipv4.ping_group_range kernel parameter is not set, then we fall back to trying to create a RAW socket to send ICMP packets. This usually requires "root" privileges, so it's likely to fail. If all else fails, fall back to TCP and implement tcp echo */ Also, is there any scope for simplifying the comments a bit so that they could be shortened to 2-3 line comments? For example // Check OS supports ICMP. If IPPROTO_ICMP is not present or // the net.ipv4.ping_group_range kernel parameter is not set, fall // back to trying to create a RAW socket to send ICMP packets. // If all else fails, fall back to TCP and implement tcp echo. The second comment is more of a suggestion concerning formatting and not what I think should be present. Having simple and concise comments is always beneficial for future maintenance in my opinion. - PR: https://git.openjdk.java.net/jdk/pull/1502
Integrated: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner
On Mon, 7 Dec 2020 09:52:47 GMT, Conor Cleary wrote:
> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously
> seen to fail with an IllegalMonitorStateException. This failure was caused by
> the use of `wait()` in a non synchronized block. This failure was mitigated
> through use of `await()` instead as is shown below (code can be viewed on
> L119 in
> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>
> ...
> long timeout = TIMEOUT;
> while ((kace = poll()) == null) {
> waiter.await(timeout, TimeUnit.MILLISECONDS);
> ...
> While the code throwing the exception was fixed, a regression test was not
> made. So this patch adds a simple white-box test which calls the
> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if
> not used in a synchronized block or method, so the test verifies that it is
> not used.
This pull request has now been integrated.
Changeset: f9c9bf03
Author:Conor Cleary
Committer: Daniel Fuchs
URL: https://git.openjdk.java.net/jdk/commit/f9c9bf03
Stats: 70 lines in 2 files changed: 70 ins; 0 del; 0 mod
8255583: Investigate creating a test to trigger the condition in
KeepAliveStreamCleaner
Reviewed-by: dfuchs, chegar, michaelm
-
PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v2]
On Mon, 7 Dec 2020 14:53:25 GMT, Daniel Fuchs wrote: >> Conor Cleary has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - 8255583: Added original bug id to bug tag >> - 8255583: Add bug & summary tags to KeepAliveStreamCleanerTestDriver > > test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java > line 27: > >> 25: * @test >> 26: * @bug 8255124 8255583 >> 27: * @summary Tests that KeepAliveStreamCleaner run does not throw an >> IllegalMonitorState Exception. > > Thanks for the summary! > `@bug` should only list `8255124` ; We typically don't list issues that > affect the tests only. We only list those that affect the JDK source code and > which can be verified by running the test. Understood, I removed the additional id and now only 8255124 is present. - PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v3]
> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously
> seen to fail with an IllegalMonitorStateException. This failure was caused by
> the use of `wait()` in a non synchronized block. This failure was mitigated
> through use of `await()` instead as is shown below (code can be viewed on
> L119 in
> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>
> ...
> long timeout = TIMEOUT;
> while ((kace = poll()) == null) {
> waiter.await(timeout, TimeUnit.MILLISECONDS);
> ...
> While the code throwing the exception was fixed, a regression test was not
> made. So this patch adds a simple white-box test which calls the
> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if
> not used in a synchronized block or method, so the test verifies that it is
> not used.
Conor Cleary has updated the pull request incrementally with one additional
commit since the last revision:
8255583: Removed unnecessary bug id
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/1659/files
- new: https://git.openjdk.java.net/jdk/pull/1659/files/a6ca8d04..785e66ac
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/1659.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1659/head:pull/1659
PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v2]
On Mon, 7 Dec 2020 11:09:47 GMT, Conor Cleary wrote: >> Will do Daniel, thanks for pointing that out! > > Should probably add a short summary as well Added bug ids for the original issue which you referred to and for the issue this PR relates to. Will mark as resolved if this is all good - PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v2]
> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously
> seen to fail with an IllegalMonitorStateException. This failure was caused by
> the use of `wait()` in a non synchronized block. This failure was mitigated
> through use of `await()` instead as is shown below (code can be viewed on
> L119 in
> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>
> ...
> long timeout = TIMEOUT;
> while ((kace = poll()) == null) {
> waiter.await(timeout, TimeUnit.MILLISECONDS);
> ...
> While the code throwing the exception was fixed, a regression test was not
> made. So this patch adds a simple white-box test which calls the
> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if
> not used in a synchronized block or method, so the test verifies that it is
> not used.
Conor Cleary has updated the pull request incrementally with two additional
commits since the last revision:
- 8255583: Added original bug id to bug tag
- 8255583: Add bug & summary tags to KeepAliveStreamCleanerTestDriver
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/1659/files
- new: https://git.openjdk.java.net/jdk/pull/1659/files/ebbe000e..a6ca8d04
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=00-01
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/1659.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1659/head:pull/1659
PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner
On Mon, 7 Dec 2020 10:55:06 GMT, Daniel Fuchs wrote:
>> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously
>> seen to fail with an IllegalMonitorStateException. This failure was caused
>> by the use of `wait()` in a non synchronized block. This failure was
>> mitigated through use of `await()` instead as is shown below (code can be
>> viewed on L119 in
>> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>>
>> ...
>> long timeout = TIMEOUT;
>> while ((kace = poll()) == null) {
>> waiter.await(timeout, TimeUnit.MILLISECONDS);
>> ...
>> While the code throwing the exception was fixed, a regression test was not
>> made. So this patch adds a simple white-box test which calls the
>> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if
>> not used in a synchronized block or method, so the test verifies that it is
>> not used.
>
> test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java
> line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @modules java.base/sun.net.www.http
>
> Hi Conor,
>
> Can you add:
>
> @bug 8255124
>
> here? [8255124](https://bugs.openjdk.java.net/browse/JDK-8255124) is the
> issue whose fix this test is verifying.
>
> best regards,
>
> -- daniel
Will do Daniel, thanks for pointing that out!
-
PR: https://git.openjdk.java.net/jdk/pull/1659
Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner
On Mon, 7 Dec 2020 11:08:37 GMT, Conor Cleary wrote: >> test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java >> line 26: >> >>> 24: /* >>> 25: * @test >>> 26: * @modules java.base/sun.net.www.http >> >> Hi Conor, >> >> Can you add: >> >> @bug 8255124 >> >> here? [8255124](https://bugs.openjdk.java.net/browse/JDK-8255124) is the >> issue whose fix this test is verifying. >> >> best regards, >> >> -- daniel > > Will do Daniel, thanks for pointing that out! Should probably add a short summary as well - PR: https://git.openjdk.java.net/jdk/pull/1659
RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner
The KeepAliveStreamCleaner in sun.net.ww.http package had been previously seen
to fail with an IllegalMonitorStateException. This failure was caused by the
use of `wait()` in a non synchronized block. This failure was mitigated through
use of `await()` instead as is shown below (code can be viewed on L119 in
src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
...
long timeout = TIMEOUT;
while ((kace = poll()) == null) {
waiter.await(timeout, TimeUnit.MILLISECONDS);
...
While the code throwing the exception was fixed, a regression test was not
made. So this patch adds a simple white-box test which calls the
KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if not
used in a synchronized block or method, so the test verifies that it is not
used.
-
Commit messages:
- 8255583: Updated copyright notes
- 8255583: Investigate creating a test to trigger the condition in
KeepAliveStreamCleaner
Changes: https://git.openjdk.java.net/jdk/pull/1659/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8255583
Stats: 49 lines in 2 files changed: 42 ins; 6 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/1659.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1659/head:pull/1659
PR: https://git.openjdk.java.net/jdk/pull/1659
Integrated: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 12:46:08 GMT, Conor Cleary wrote: > The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. This pull request has now been integrated. Changeset: f5d36e6c Author:Conor Cleary Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/f5d36e6c Stats: 83 lines in 1 file changed: 17 ins; 39 del; 27 mod 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed Reviewed-by: chegar, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v2]
On Thu, 5 Nov 2020 16:30:31 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8246741: Corrected summary tag, moved record declaration
>
> test/jdk/java/net/NetworkInterface/UniqueMacAddressesTest.java line 46:
>
>> 44:
>> 45: static PrintStream log = System.err;
>> 46: record NetIfPair(String interfaceName, byte[] address) {}
>
> Nit: maybe add a blank line, and a comment:
> static PrintStream log = System.err;
>
> // A record pair (NetworkInterface::name,
> NetworkInterface::hardwareAddress)
> record NetIfPair(String interfaceName, byte[] address) {}
I agree, this is more clear for some one new looking at the test. Will add in
that comment now
-
PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v3]
> The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8246741: Added descriptive comment to record - Changes: - all: https://git.openjdk.java.net/jdk/pull/1076/files - new: https://git.openjdk.java.net/jdk/pull/1076/files/1869acc6..770455f3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=01-02 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed [v2]
> The primary goal of this fix was to refactor > NetworkInterface/UniqueMacAddressesTest.java to make use of the > jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. > This was due to the original failure being related to the use of the 'awdl' > interface on macOS, which should be skipped for functional tests. The > NetworkConfiguration class accounts for this when returning the list of > present interfaces (originally retrieved using > NetworkInterface.getNetworkInterfaces()) as well as for other OS specific > properties. The effects of these changes can be seen on L59 and L101-106. > > In addition to these changes, some modifications were made to the test in the > interest of keeping it concise and readable but were made bearing in mind the > original logging of the test was already up to a good standard (and I sought > to keep it that way). To summarize the other changes: > > - When using NetworkConfiguration::interfaces to read in a stream of > NetworkInterface objects, the objects are collected into a list of records > (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to > NetworkInterface::getHardwareAddress (which can throw a SocketException) > throughout the test. > - When checking if the MAC addresses are unique, the testMacAddressesEqual() > method is not called unless the interface names are different. If > testMacAddressesEqual() returns true, the calling method will simply return > instead of updating the value of a boolean variable. > - Majority of logging was moved to the testMacAddressesEqualMethod() so that > only logs for comparisons are carried out. There is scope for additional > logging of, for example, the list of present interfaces returned by > createNetworkInterfaceList() if deemed necessary by review. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8246741: Corrected summary tag, moved record declaration - Changes: - all: https://git.openjdk.java.net/jdk/pull/1076/files - new: https://git.openjdk.java.net/jdk/pull/1076/files/3e086804..1869acc6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Re: RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
On Thu, 5 Nov 2020 14:10:15 GMT, Patrick Concannon wrote: > Great work, Conor. The test looks much better! > > Did you consider using testNG? Might help you condense things a bit more Thanks @pconcannon! I definitely do prefer testNG but was keen not to venture any further away from the scope of the fix than I may already have done with the other modifications. So with that in mind I tried to maintain the same test behavior i.e. throw a RuntimeException on failure instead of some sort of testNG assertion error. That being said, I am open to updating the test further to make use of testNG if additional feedback warrants it. - PR: https://git.openjdk.java.net/jdk/pull/1076
RFR: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
The primary goal of this fix was to refactor NetworkInterface/UniqueMacAddressesTest.java to make use of the jdk.test.lib.NetworkConfiguration class instead of java.net.NetworkInterface. This was due to the original failure being related to the use of the 'awdl' interface on macOS, which should be skipped for functional tests. The NetworkConfiguration class accounts for this when returning the list of present interfaces (originally retrieved using NetworkInterface.getNetworkInterfaces()) as well as for other OS specific properties. The effects of these changes can be seen on L59 and L101-106. In addition to these changes, some modifications were made to the test in the interest of keeping it concise and readable but were made bearing in mind the original logging of the test was already up to a good standard (and I sought to keep it that way). To summarize the other changes: - When using NetworkConfiguration::interfaces to read in a stream of NetworkInterface objects, the objects are collected into a list of records (see [JEP 395](https://openjdk.java.net/jeps/395)) to avoid multiple calls to NetworkInterface::getHardwareAddress (which can throw a SocketException) throughout the test. - When checking if the MAC addresses are unique, the testMacAddressesEqual() method is not called unless the interface names are different. If testMacAddressesEqual() returns true, the calling method will simply return instead of updating the value of a boolean variable. - Majority of logging was moved to the testMacAddressesEqualMethod() so that only logs for comparisons are carried out. There is scope for additional logging of, for example, the list of present interfaces returned by createNetworkInterfaceList() if deemed necessary by review. - Commit messages: - 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed Changes: https://git.openjdk.java.net/jdk/pull/1076/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1076&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246741 Stats: 83 lines in 1 file changed: 16 ins; 39 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1076/head:pull/1076 PR: https://git.openjdk.java.net/jdk/pull/1076
Integrated: 8253179: Replace LinkedList Impl in net.http.Http2Connection
On Wed, 30 Sep 2020 10:22:11 GMT, Conor Cleary wrote: > This patch replaces a LinkedList data structure used in the > net.http.Http2Connection class with an ArrayList. This > issue relates to [JDK-8246048: Replace LinkedList with ArrayLists in > java.net](https://bugs.openjdk.java.net/browse/JDK-8246048). Some > justifications for this change are as follows: > > - Sequential Access Times for ArrayLists are improved due to locality of > reference (i.e ArrayList elements stored in same > memory neighborhood) > - Get(index) operations are O(1) time complexity for ArrayLists as opposed to > worst-case O(N-1) for LinkedLists > - While insertion operations can be expensive (O(N) in the worst case), these > operations appear to be > infrequent/non-existent in this case. > > Additional justifications or challenges to those listed are welcome! The > general idea is that ArrayLists out-perform > LinkedLists in this scenario. This pull request has now been integrated. Changeset: 703b345e Author:Conor Cleary Committer: Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/703b345e Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod 8253179: Replace LinkedList Impl in net.http.Http2Connection Reviewed-by: dfuchs, prappo, chegar, shade - PR: https://git.openjdk.java.net/jdk/pull/431
Re: RFR: 8253179: Replace LinkedList Impl in net.http.Http2Connection [v2]
On Tue, 6 Oct 2020 17:04:01 GMT, Aleksey Shipilev wrote: > I can sponsor this, but it is not clear what testing was done on this. At > least `tier1`? `tier1`, `tier2`, `jdk_net`, `test/jdk/java/net/httpclient` tests were conducted with all passing. - PR: https://git.openjdk.java.net/jdk/pull/431
Re: RFR: 8253179: Replace LinkedList Impl in net.http.Http2Connection [v2]
On Fri, 2 Oct 2020 08:41:56 GMT, Conor Cleary wrote:
>> This patch replaces a LinkedList data structure used in the
>> net.http.Http2Connection class with an ArrayList. This
>> issue relates to [JDK-8246048: Replace LinkedList with ArrayLists in
>> java.net](https://bugs.openjdk.java.net/browse/JDK-8246048). Some
>> justifications for this change are as follows:
>>
>> - Sequential Access Times for ArrayLists are improved due to locality of
>> reference (i.e ArrayList elements stored in same
>> memory neighborhood)
>> - Get(index) operations are O(1) time complexity for ArrayLists as opposed
>> to worst-case O(N-1) for LinkedLists
>> - While insertion operations can be expensive (O(N) in the worst case),
>> these operations appear to be
>> infrequent/non-existent in this case.
>>
>> Additional justifications or challenges to those listed are welcome! The
>> general idea is that ArrayLists out-perform
>> LinkedLists in this scenario.
>
> Conor Cleary has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8253179: Removed ArrayList copy of streams
src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line
704:
> 702: client2.deleteConnection(this);
> 703: for (Stream s : streams.values()) {
> 704: try {
To address some of the feedback given, the streams are now directly addressed
instead of making a copy. ConcurrentMap
has also been used to initialise streams instead of Map. Seeking
feedback/opinions on this if possible
-
PR: https://git.openjdk.java.net/jdk/pull/431
Re: RFR: 8253179: Replace LinkedList Impl in net.http.Http2Connection [v2]
> This patch replaces a LinkedList data structure used in the > net.http.Http2Connection class with an ArrayList. This > issue relates to [JDK-8246048: Replace LinkedList with ArrayLists in > java.net](https://bugs.openjdk.java.net/browse/JDK-8246048). Some > justifications for this change are as follows: > > - Sequential Access Times for ArrayLists are improved due to locality of > reference (i.e ArrayList elements stored in same > memory neighborhood) > - Get(index) operations are O(1) time complexity for ArrayLists as opposed to > worst-case O(N-1) for LinkedLists > - While insertion operations can be expensive (O(N) in the worst case), these > operations appear to be > infrequent/non-existent in this case. > > Additional justifications or challenges to those listed are welcome! The > general idea is that ArrayLists out-perform > LinkedLists in this scenario. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8253179: Removed ArrayList copy of streams - Changes: - all: https://git.openjdk.java.net/jdk/pull/431/files - new: https://git.openjdk.java.net/jdk/pull/431/files/9ecb85ea..fc3f574d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=431&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=431&range=00-01 Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/431/head:pull/431 PR: https://git.openjdk.java.net/jdk/pull/431
Re: RFR: 8253179: Replace LinkedList Impl in net.http.Http2Connection
On Thu, 1 Oct 2020 09:43:46 GMT, Daniel Fuchs wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java >> line 703: >> >>> 701: if (initialCause == null) this.cause = t; >>> 702: client2.deleteConnection(this); >>> 703: List> c = new ArrayList<>(streams.values()); >> >> Why can't we dispense with this copy completely, and instead just iterate >> `streams.values()`? > > Good point Aleksey. I guess that the original intent was to avoid > `ConcurrentModificationException`. However I see that > `streams` is a `ConcurrentHashMap` now. So maybe we could dispense of the > copy: > `private final Map> streams = new > ConcurrentHashMap<>();` > > Could be worth changing the type from `Map>` to > `ConcurrentMap>` in the declaration > above if we decide to get rid of the copy though. Taking a look at this presently - PR: https://git.openjdk.java.net/jdk/pull/431
RFR: 8253179: Replace LinkedList Impl in net.http.Http2Connection
This patch replaces a LinkedList data structure used in the net.http.Http2Connection class with an ArrayList. This issue relates to [JDK-8246048: Replace LinkedList with ArrayLists in java.net](https://bugs.openjdk.java.net/browse/JDK-8246048). Some justifications for this change are as follows: - Sequential Access Times for ArrayLists are improved due to locality of reference (i.e ArrayList elements stored in same memory neighborhood) - Get(index) operations are O(1) time complexity for ArrayLists as opposed to worst-case O(N-1) for LinkedLists - While insertion operations can be expensive (O(N) in the worst case), these operations appear to be infrequent/non-existent in this case. Additional justifications or challenges to those listed are welcome! The general idea is that ArrayLists out-perform LinkedLists in this scenario. - Commit messages: - 8253179: Replace LinkedList Impl in net.http.Http2Connection Changes: https://git.openjdk.java.net/jdk/pull/431/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=431&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253179 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/431/head:pull/431 PR: https://git.openjdk.java.net/jdk/pull/431
Re: RFR[8246047]: 'Replace LinkedList impl in net.http.websocket.BuilderImpl'
Thanks for the suggestion Daniel, looks much nicer without the casting in the immutableCopy function as well! I made a new patch with those reverted changes and it looks much nicer and still behaves as expected. webrev: http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8246047/webrevs/webrev.03/ Thanks! Conor On 14/08/2020 15:49, Daniel Fuchs wrote: Hi Conor, Thanks for addressing this technical debt. I don't think this is correct. You transformed the immutable copy at line 137 into a shallow clone, since now both copies share the same mutable list. I suggest to revert the changes at lines: 66,67,144 and 145. (and that will be much simpler in the bargain :-) ) best regards, -- daniel On 14/08/2020 15:37, Conor Cleary wrote: Hi all, Requesting some reviewers for a patch concerning JDK-8246047 'Replace LinkedList impl in net.http.websocket.BuilderImpl'. This patch replaces LinkedList data structures used in the net.http.websocket BuilderImpl class with ArrayLists. In particular, the 'headers' and 'subprotocols' Collections in the class are now assigned ArrayLists in the BuilderImpl constructors. Some justifications for this change are as follows: * Sequential Access Times for ArrayLists are improved due to locality of reference (i.e ArrayList elements stored in same neighbourhood) * Get(index) operations are O(1) time complexity for ArrayLists as opposed to worst-case O(N-1) for LinkedLists * While insertion operations can be expensive (O(N) in the worst case), the 'headers' and 'subprotocols' lists are not modified after creation and so this is of minor concern Additional justifications or challenges to them are welcome! The general idea is that ArrayLists out-perform LinkedLists in this particular scenario. * webrev: http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8246047/webrevs/webrev.02/ * bug: https://bugs.openjdk.java.net/browse/JDK-8246047 Conor
Re: RFR[8246047]: 'Replace LinkedList impl in net.http.websocket.BuilderImpl'
Hi all, Requesting some reviewers for a patch concerning JDK-8246047 'Replace LinkedList impl in net.http.websocket.BuilderImpl'. This patch replaces LinkedList data structures used in the net.http.websocket BuilderImpl class with ArrayLists. In particular, the 'headers' and 'subprotocols' Collections in the class are now assigned ArrayLists in the BuilderImpl constructors. Some justifications for this change are as follows: * Sequential Access Times for ArrayLists are improved due to locality of reference (i.e ArrayList elements stored in same neighbourhood) * Get(index) operations are O(1) time complexity for ArrayLists as opposed to worst-case O(N-1) for LinkedLists * While insertion operations can be expensive (O(N) in the worst case), the 'headers' and 'subprotocols' lists are not modified after creation and so this is of minor concern Additional justifications or challenges to them are welcome! The general idea is that ArrayLists out-perform LinkedLists in this particular scenario. * webrev: http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8246047/webrevs/webrev.02/ * bug: https://bugs.openjdk.java.net/browse/JDK-8246047 Conor
RFR[8246143]: 'UnreferencedXXX tests fail when run with --illegal-access=deny'
Hi, Could someone please take a look at my webrev for JDK-8246143: 'UnreferencedXXX tests fail when run with --illegal-access=deny'? This patch addresses test failures with test/jdk/java/net/DatagramSocket/UnreferencedDatagramSockets.java and test/jdk/java/net/MulticastSocket/UnreferencedMulticastSockets.java when run with --illegal-access=deny. This revision addresses this failure in both tests by adding java.base/sun.nio.ch:+open to the modules tag. An additional test run is also with the --illegal-access=deny configuration set. bug: https://bugs.openjdk.java.net/browse/JDK-8246143 webrev: http://cr.openjdk.java.net/~pconcannon/ccleary/8246143/webrevs/webrev.00/ Regards, Conor
