Re: RFR: 8286962: java/net/httpclient/ServerCloseTest.java failed once with ConnectException
On Tue, 14 Jun 2022 15:20:28 GMT, Conor Cleary wrote:
> **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.
Thanks for the fix Conor! I have suggested some changes that should make the
fix easier to review and keep it closer to the original.
test/jdk/java/net/httpclient/ServerCloseTest.java line 302:
> 300:
> 301: StringTokenizer tokenizer = new
> StringTokenizer(requestLine);
> 302: tokenizer.nextToken(); // Skip method token as not
> used
Since we were asserting here before, maybe we could directly close the
clientConnection here if `method` is not GET or POST.
Something like:
if (!"GET".equals(method) && !POST.equals(method)) {
System.out.println(now() + getName() + ": Unexpected method: " + method);
clientConnection.close();
continue;
}
test/jdk/java/net/httpclient/ServerCloseTest.java line 304:
> 302: tokenizer.nextToken(); // Skip method token as not
> used
> 303: String path = tokenizer.nextToken();
> 304:
We should check `path` immediately before trying to build the URI. If it
doesn't contain what we expect, then close the clientConnection and continue as
suggested above.
test/jdk/java/net/httpclient/ServerCloseTest.java line 313:
> 311: System.err.printf("Bad target address: \"%s\" in
> \"%s\"%n",
> 312: path, requestLine);
> 313: validURI = false;
maybe just leave that as before?
test/jdk/java/net/httpclient/ServerCloseTest.java line 317:
> 315:
> 316: // Proceed if URI is well-formed and the request
> path is as expected
> 317: if (validURI && path.contains("/dummy/x")) {
Since we will have already checked `path`, `method` and `uri` here just
unconditionally add the clientConnection to the list and proceed as before.
-
Changes requested by dfuchs (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9155
Re: HTTP/308 handling in HttpURLConnection
Hi Robert, On 10/06/2022 11:54, Robert Stupp wrote: Does anyone recall why HTTP/308 was not implemented that time? I suspect that HTTP/308 was just not a thing 14/15 years ago, but no clue whether it’s just not been implemented all the time or whether there was a reason to not handle it at all in HttpURLConnection. I could not find anything useful in the OpenJDK commit log nor in the OpenJDK JIRA (there was a ticket about the Webstart client). The history you can observe probably dates back to the time when Java was open sourced, but the networking code is probably older than that. I would guess that 308 was never implemented. Do you have an account for https://bugs.openjdk.org/ ? If you do please log an RFE (component core-libs, subcomponent java.net) and we will evaluate it. Otherwise you can log it through https://bugreport.java.com/ I see that 308 is not very different from 301 except that it explicitly prevents turning a POST into a GET on redirection - so there may be several places to tweak in the HttpURLConnection code if we decided to implement this. best regards, -- daniel
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v4]
On Thu, 9 Jun 2022 13:06:34 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. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8286171: Updated comment LGTM Conor. Please integrate when you are ready and drop me a note: I will sponsor. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9093
Re: RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v3]
On Thu, 9 Jun 2022 12:05:43 GMT, Jaikiran Pai wrote: >> 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. > > From what I understand, the HTTP version in the client request comes in as an > input to the test method and we pass `HTTP/1.1` as one such version. In that > case there won't be a HTTP/2 upgrade involved since the request (via the > client) will be pinned to HTTP/1.1 version. The test will still pass, but it > won't be testing any `Expect: 100-continue` with `GET` requests. Of course > GET requests aren't expected to have a request body, so testing this header > with that method may not be meaningful after all (although the client > implementation doesn't enforce that restriction). The GET request is only needed when the input version is HTTP/2 - we don't want to mix the upgrade with 100-continue. I am not sure how well our HTTP/2 server supports that. Our client would expect the server to first send 100-continue, and then do the upgrade after receiving the request body through HTTP/1.1. We don't support the other way round (101 followed by 100). Here again maybe we should log a followup to try and verify what happens when both upgrade + 100 are specified - but for the moment I'll be satisfied if our client no longer blocks when receiving 417 (or any other code). - PR: https://git.openjdk.org/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:26:52 GMT, Jaikiran Pai wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 308: >> >>> 306: // Have to mark request as sent, due to no request body being >>> sent >>> 307: // in the event of a 417 Expectation Failed >>> 308: requestSent(); >> >> The `requestSent()` method in `Stream` class sets a flag and only closes the >> Stream instance if the `responseReceived` is also set. As far as I can see >> in the `Stream` code, that flag plays no other major role and doesn't >> "trigger" any kind of action (I might be wrong though). Here, are we >> expecting the caller client to get a request failure (just like the >> `HTTP/1.1` case)? If so, should we instead of calling >> `Stream.completeResponseExceptionally(Throwable)` or something similar which >> completes the `CompletableFuture`? > > I think I see what we are doing here. The `Exchange` change that we did in > this PR marks the `CompletableFuture` as completed with that response that > was received. So the caller will see the future as completed. So the only > missing piece for me now is, will the `Stream` get closed in this case, with > the current change in the PR? We need to close the stream after the exchange has terminated, but the exchange will not be considered to be terminated if we have a request body and the request body has not been sent. So we are actually pretending that the request body has been sent here in order for the stream to get gracefully closed after the final (417) response has been received. - 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:10:11 GMT, Jaikiran Pai wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8286171: Added teardown method and comments
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line
> 862:
>
>> 860: // Sets a flag which closes the connection locally when
>> 861: // onFinished() is called
>> 862: response.closeWhenFinished();
>
> I checked the HTTP/1.1 spec to see what it says about the `100-Continue`
> request/response flow. The spec (here
> https://www.rfc-editor.org/rfc/rfc7231#section-5.1.1) from a server
> perspective says that the server might either return a `417` or a final
> status code (or of course `100`). A 4xx is considered a client error and I
> think we don't close a connection on 4xx errors (I haven't looked thoroughly
> at the code yet to verify that part). So is there a reason why we would want
> to close a connection if the server responds with `417`? In fact, the spec
> says this for clients:
>
>
> A client that receives a 417 (Expectation Failed) status code in
> response to a request containing a 100-continue expectation SHOULD
> repeat that request without a 100-continue expectation, since the
> 417 response merely indicates that the response chain does not
> support expectations (e.g., it passes through an HTTP/1.0 server).
>
>
> It says `SHOULD`, so it isn't mandated that we retry the request on `417`.
>
> Furthermore, the spec states that if the server sends a final status code
> instead of 417, then it might also send a `Connection: close` header to
> indicate whether or not the connection should be closed:
>
>
> A server that responds with a final status code before reading the
> entire message body SHOULD indicate in that response whether it
> intends to close the connection or continue reading and discarding
> the request message (see Section 6.6 of [RFC7230]).
>
>
> Are we closing the connection irrespective of the status code (and other
> headers) to keep the implementation simpler? If that's the reason, then I
> think that's fine, since it still is within what the spec seems to allow. I'm
> just curious if there's some other reason for doing this.
All good points. Yes - we could make a special case for 417 - and maybe we
should log another issue to do that. We choose to close the connection in all
cases here to make the implementation simpler. In practice I guess we could
leave the connection opened - but if a server sends back anything but 100 then
we might not be entirely sure of what state the server is in, so closing the
connection seems safer.
> 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!
-
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 10:31:31 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.
>
> Conor Cleary has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8286171: Added teardown method and comments
test/jdk/java/net/httpclient/ExpectContinueTest.java line 286:
> 284: resp = cf.join();
> 285: System.err.println("Response Headers: " + resp.headers());
> 286: System.err.println("Response Status Code: " + resp.statusCode());
Would be good to add some `assertEquals` here to verify that the status codes
we get are those that we expect. Thanks!
-
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. test/jdk/java/net/httpclient/ExpectContinueTest.java line 2: > 1: /* > 2: * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights > reserved. Copyright line should be: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. - 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. Good fix! Still some issues with the test. src/java.net.http/share/classes/jdk/internal/net/http/ExchangeImpl.java line 255: > 253: > 254: // Called when server returns non 100 response to > 255: // and Expect-Continue Typo? test/jdk/java/net/httpclient/ExpectContinueTest.java line 89: > 87: InetSocketAddress saHang = new > InetSocketAddress(InetAddress.getLoopbackAddress(), 0); > 88: > 89: httpTestServer = > HttpServerAdapters.HttpTestServer.of(HttpServer.create(sa, 0)); You should be able to write: httpTestServer = HttpTestServer.of(HttpServer.create(sa, 0)); here. No need for the `HttpServerAdapters` prefix. test/jdk/java/net/httpclient/ExpectContinueTest.java line 217: > 215: os.write(bytes); > 216: os.flush(); > 217: } maybe you should close the client socket if validRequest == false. You should also implement a close() method to close the server socket and the client socket when the test terminates. Also the test seems to be missing a teardown() method to properly close and stop all the servers. - PR: https://git.openjdk.java.net/jdk/pull/9093
Re: RFR: 8276798: HttpURLConnection sends invalid HTTP request
On Mon, 6 Jun 2022 09:43:50 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix > https://bugs.openjdk.java.net/browse/JDK-8276798? > > `sun.net.www.protocol.http.HttpURLConnection` has a (private) `writeRequests` > method. This method is responsible for creating the standard HTTP request > headers (include the request line) and then writing it out to the > `OutputStream` which communicates with the HTTP server. While writing out > these request headers, if there's an IOException, then `HttpURLConnection` > marks a `failedOnce` flag to `true` and attempts to write these again afresh > (after creating new connection to the server). This re-attempt is done just > once. > > As noted in the linked JBS issue, specifically this comment > https://bugs.openjdk.java.net/browse/JDK-8276798?focusedCommentId=14500074&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14500074, > there's a specific case where creating and writing out these request headers > ends up skipping the request line, which causes the server to respond back > with a "Bad Request" response code. > > The commit in this PR removes the use of `failedOnce` flag that was being > used to decide whether or not to write the request line. The existing code > doesn't have any specific comments on why this check was there in first > place, nor does the commit history show anything about this check. However, > reading through that code, my guess is that, it was there to avoid writing > the request line twice when the same `requests` object gets reused during the > re-attempt. I think a better check would be the see if the `requests` already > has the request line and if not add it afresh. > While in this code, I also removed the check where the `failedOnce` flag was > being used to check if the `Connection: Keep-Alive`/`Proxy-Connection: > Keep-alive` header needs to be set. This part of the code already has a call > to `setIfNotSet`, so I don't think we need the `failedOnce` check here. > > tier1, tier2 and tier3 tests have passed without issues. However, given the > nature of this code, I'm not too confident that we have tests which can catch > this issue (and adding one isn't easy), so I would like inputs on whether > this change is good enough or whether it has the potential to cause any > non-obvious regressions. This looks reasonable to me but I would like to get a second opinion. @Michael-Mc-Mahon ? - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9038
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Changes to `net` and `http` look good. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8272702: Resolving URI relative path with no / may lead to incorrect toString
On Thu, 26 May 2022 09:18:56 GMT, KIRIYAMA Takuya wrote:
> Consider an authority component without trailing "/" as a base URI. When
> resolving a relative path against this base URI, the resulting URI is a
> concatenated URI without "/".
> This behaviour should be fixed, which is rationalized by
> rfc3986#section-5.2.3.
> Could you review this fix?
Changes requested by dfuchs (Reviewer).
src/java.base/share/classes/java/net/URI.java line 2140:
> 2138: } else {
> 2139: sb.append("/");
> 2140: }
This is wrong as it will cause `URI.create("foo").resolve(URI.create("test"))`
to return `"/test"` instead of `"test"`
-
PR: https://git.openjdk.java.net/jdk/pull/8899
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
On Sat, 30 Apr 2022 10:17:43 GMT, Andrey Turbanov wrote: > `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` > +`put` calls. > > https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165 > > Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to > follow. We know that `requests` can contain only non-null values. LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher [v2]
On Tue, 31 May 2022 15:11:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which addresses the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8287318? >> >> The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select >> keys of interested. The selected keys is then iterated over and each key is >> removed through the iterator. This is fine as long as the selector isn't >> then used to invoke select operation(s) while the iterator is still in use. >> Doing so leads to the underlying Set being potentially modified with updates >> to the selected keys. As a result any subsequent use of the iterator will >> lead to `ConcurrentModificationException` as seen in the linked issue. >> >> The commit here fixes that by creating a copy of the selected keys and >> iterating over it so that any subsequent select operation on the selector >> won't have impact on the Set that is being iterated upon. >> >> No new tests have been added given the intermittent nature of this issue. >> Existing tier1, tier2 and tier3 tests passed without any related failures, >> after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Collection.toArray(...) instead of creating a copy of the collection Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher
On Thu, 26 May 2022 07:17:12 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which addresses the issue noted in
> https://bugs.openjdk.java.net/browse/JDK-8287318?
>
> The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select
> keys of interested. The selected keys is then iterated over and each key is
> removed through the iterator. This is fine as long as the selector isn't then
> used to invoke select operation(s) while the iterator is still in use. Doing
> so leads to the underlying Set being potentially modified with updates to the
> selected keys. As a result any subsequent use of the iterator will lead to
> `ConcurrentModificationException` as seen in the linked issue.
>
> The commit here fixes that by creating a copy of the selected keys and
> iterating over it so that any subsequent select operation on the selector
> won't have impact on the Set that is being iterated upon.
>
> No new tests have been added given the intermittent nature of this issue.
> Existing tier1, tier2 and tier3 tests passed without any related failures,
> after this change.
Marked as reviewed by dfuchs (Reviewer).
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 384:
> 382: final Set copy = new
> HashSet<>(selected);
> 383: // iterate over the copy
> 384: for (final SelectionKey key : copy) {
Another possibility would be to call toArray() - since we're simply going to
iterate we don't need a full copy of the hashset - e.g.: `for (var key :
selected.toArray(SelectionKey[]::new)) {`
-
PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287104: AddressChangeListener thread inherits CCL and can cause memory leak for webapp-servers [v3]
On Wed, 25 May 2022 09:30:46 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which addresses >> https://bugs.openjdk.java.net/browse/JDK-8287104? >> >> The change in this commit now uses an `InnocuousThread` to create a thread >> with `null` context classloader. The `Runnable` task of this thread just >> invokes a native method through JNI to be notified of IP addresses change of >> the host. As such any specific thread context classloader isn't necessary in >> this thread. >> >> Additionally, this commit does some minor changes like making the `lock` >> member variable `final` and also marking the `changed` member variable as >> `volatile`. These changes aren't necessary for this fix, but I think would >> be good to have while we are changing this part of the code. >> >> Finally, the thread that we create here, now has a specific name >> `Net-address-change-listener` instead of the usual system wide >> auto-generated name. >> >> No new tests have been added for this change. Existing tier1, tier2 and >> tier3 tests have been run and no related failures have been noticed. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > No need for volatile as spotted by Daniel LGTM now! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8827
Re: RFR: 8287104: AddressChangeListener thread inherits CCL and can cause memory leak for webapp-servers [v2]
On Mon, 23 May 2022 12:31:40 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which addresses >> https://bugs.openjdk.java.net/browse/JDK-8287104? >> >> The change in this commit now uses an `InnocuousThread` to create a thread >> with `null` context classloader. The `Runnable` task of this thread just >> invokes a native method through JNI to be notified of IP addresses change of >> the host. As such any specific thread context classloader isn't necessary in >> this thread. >> >> Additionally, this commit does some minor changes like making the `lock` >> member variable `final` and also marking the `changed` member variable as >> `volatile`. These changes aren't necessary for this fix, but I think would >> be good to have while we are changing this part of the code. >> >> Finally, the thread that we create here, now has a specific name >> `Net-address-change-listener` instead of the usual system wide >> auto-generated name. >> >> No new tests have been added for this change. Existing tier1, tier2 and >> tier3 tests have been run and no related failures have been noticed. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Aleksei's review suggestion - use a better Thread name src/java.base/windows/classes/sun/net/dns/ResolverConfigurationImpl.java line 49: > 47: // Addresses have changed. We default to true to make sure we > 48: // resolve the first time it is requested. > 49: private static volatile boolean changed = true; The field `changed` is only accessed when `synchronized` on `lock` - except for this initialization line here. I wonder if the volatile is actually needed. A better fix might be to rename it to `nochanges` and revert its semantics, which would allow to leave it initialized to false. Or maybe just revert changes to line 49. Otherwise looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/8827
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Looks ok-ish. The idea of starting with a small timeout is a good one. I am a bit less sure about moving the post-asserts inside the loop - or closing before asserting that the cf hangs, but if I understand the logic correctly it seems ok too. Maybe give some time for @pavelrappo to chime in before integrating. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]
On Mon, 16 May 2022 08:59:43 GMT, Conor Cleary wrote: >> **Issue** >> When using the `HttpClient.send()` to send a GET request created using the >> `HttpRequest.newBuilder()`, a `Content-length: 0` header is set. This >> behaviour causes issues with many services as a body related header is >> usually not expected to be included with a GET request. >> >> **Solution** >> `Http1Request.java` was modified so that when the request method is a GET, a >> `Content-length` header is not added to the request. However, if a developer >> chooses to include a body in a GET request (though it is generally >> considered bad practice), a `Content-length` header with the appropriate >> value will be added. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8283544: Added in missing case LGTM. If the test is stable you can integrate. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6690
Integrated: 8286194: ExecutorShutdown test fails intermittently
On Thu, 5 May 2022 19:03:13 GMT, Daniel Fuchs wrote: > Hi, please find here a patch that solves a rare intermittent test failure > observed in the test `java/net/httpclient/ExecutorShutdown.java` > > A race condition coupled with some too eager synchronization was causing a > deadlock between an Http2Connection close, a thread trying to shutdown the > HttpClient due to a RejectedTaskException, and the SelectorManager thread > trying to exit. > > The fix comprises several cleanup - in particular: > > - `Http2Connection`: no need to try to send a `GOAWAY` frame if the > underlying TCP connection is already closed > - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will > request more data from upstream if the sequential scheduler that is supposed > to handle that data once it arrives is already closed > - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an > exception is raised before `onSubscribe()` has been called > - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks > when not necessary > - `ReferenceTracker`: better thread dumps in case where the selector is still > alive at the end of the test (remove the limit that limited the stack traces > to 8 element max by no longer relying on `ThreadInfo::toString`) This pull request has now been integrated. Changeset: 04df8b74 Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/04df8b74379c9de7b20931fea1642f82569d3a2d Stats: 138 lines in 7 files changed: 110 ins; 3 del; 25 mod 8286194: ExecutorShutdown test fails intermittently Reviewed-by: jpai, michaelm - PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v3]
On Fri, 13 May 2022 11:09:31 GMT, Michael McMahon wrote: > Question about the code copied in from ThreadInfo. Is there any way we could > request a change to that class to adjust the number of stack frames printed? Thanks Michael. I have logged https://bugs.openjdk.java.net/browse/JDK-8286720 - PR: https://git.openjdk.java.net/jdk/pull/8562
Integrated: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http This pull request has now been integrated. Changeset: 5ff1d227 Author: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/5ff1d227bb878efda6262b183dfc5a0be2ce00c3 Stats: 27 lines in 3 files changed: 14 ins; 0 del; 13 mod 8286386: Address possibly lossy conversions in java.net.http Reviewed-by: rriggs, michaelm, prappo - PR: https://git.openjdk.java.net/jdk/pull/8656
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 Yes - please fix as the current proposal is buggy. - 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 Please make sure the test is stable with these changes, and if it is then LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v9]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Typo - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/1f0902ed..258a5897 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v8]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Move assert - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/25c24d67..1f0902ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=06-07 Stats: 4 lines in 1 file changed: 2 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v7]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Pavel's feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/deb22998..25c24d67 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=05-06 Stats: 8 lines in 2 files changed: 4 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v7]
On Thu, 12 May 2022 11:41:04 GMT, Pavel Rappo wrote: >> OK - I will change codeLengthOf as suggested. It was not immediately >> obvious to me that the values would fit in the first 31 bits. > >> OK - I will change codeLengthOf as suggested. It was not immediately obvious >> to me that the values would fit in the first 31 bits. > > In fact, it would even fit into the first 30 bits. There's a top-level > comment that explains the layout of `code` elements. Maybe you can improve it > somehow or carry over that specific part about the length into > `codeLengthOf`. Alternatively, you can always slap `assert` in `codeLengthOf`. Did I understand correctly that the lower 32 bits is expected to contain a length coded on 5 bits - which is expected to be a value in (5..30)? - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: lengthOfCode() returns int; Removed excessive comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/2334b747..deb22998 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=04-05 Stats: 16 lines in 2 files changed: 2 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:15:19 GMT, Pavel Rappo wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert adding char constants
>
> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
> line 291:
>
>> 289:
>> 290: HeaderWriter noMask() {
>> 291: // The negation "~" sets the high order bits
>
> Rubber-stamping this in front of every of the four closely sitting casts
> seems excessive:
>
> // The negation "~" sets the high order bits
> // so the value is more than 16 bits and the
> // compiler will emit a warning if not cast
I don't disagree. I had the same feeling. But I don't necessarily read a file
from top to bottom - and someone just glancing at one of these methods might
wonder why one line has the cast and the other hasn't.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:00:37 GMT, Pavel Rappo wrote:
>> This is what I mean:
>>
>> jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1
>> codeLengthOf ==> 2147483648
>>
>> jshell> int bufferLen = 0
>> bufferLen ==> 0
>>
>> jshell> bufferLen + codeLengthOf <= 64
>> $3 ==> false
>>
>> jshell> bufferLen + (int)codeLengthOf <= 64
>> $4 ==> true
>
> Yes, inserting explicit casts seems less clean than changing `codeLengthOf`
> to this:
>
> private static int codeLengthOf(char c) {
> return (int) (codes[c] & 0xL);
> }
>
> There are 256 elements in constant `long[] codes`. One could easily check
> that each element when ANDed with `0xL` results in a value
> that fits into the first 31 bits of `int`.
OK - I will change codeLengthOf as suggested. It was not immediately obvious
to me that the values would fit in the first 31 bits.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 08:42:39 GMT, Daniel Fuchs wrote: >> codeLengthOf() returns long. It could be changed to return int with a cast >> internally and then you could avoid the two new casts. > > No because the int returned could be negative, while the long will not. > Assuming bufferLen is 0 and codeLengthOf() returns some value that has the > 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + > codeLengthOf() will be a positive long > 64, and we won't enter the `if` here > but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be > negative and the `if` would be wrongly entered. I am not 100% sure this is a > scenario that might occur (codeLengthOf() returning large "unsigned int" > values) - but I'd prefer to stay on the safe side and assume that it can. This is what I mean: jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1 codeLengthOf ==> 2147483648 jshell> int bufferLen = 0 bufferLen ==> 0 jshell> bufferLen + codeLengthOf <= 64 $3 ==> false jshell> bufferLen + (int)codeLengthOf <= 64 $4 ==> true - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 20:31:00 GMT, Michael McMahon wrote: >> I believe the method returns an "unsigned int" - having the method return an >> int would then potentially cause `bufferLen + len <= 64` to yield true when >> it shouldn't. Hopefully @pavelrappo will comment. > > codeLengthOf() returns long. It could be changed to return int with a cast > internally and then you could avoid the two new casts. No because the int returned could be negative, while the long will not. Assuming bufferLen is 0 and codeLengthOf() returns some value that has the 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + codeLengthOf() will be a positive long > 64, and we won't enter the `if` here but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be negative and the `if` would be wrongly entered. I am not 100% sure this is a scenario that might occur (codeLengthOf() returning large "unsigned int" values) - but I'd prefer to stay on the safe side and assume that it can. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/fbf3c9a1..2334b747 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
On Wed, 11 May 2022 18:25:00 GMT, Daniel Fuchs wrote: >> @RogerRiggs Actually - I'm no longer sure that this will work: >> >> >> jshell> char x = 0b1000_ >> x ==> '耀' >> >> jshell> var y = ~x >> y ==> -32769 >> >> jshell> char y = ~x >> | Error: >> | incompatible types: possible lossy conversion from int to char >> | char y = ~x; >> | ^^ > > So if x is a char, ~x seems to be an int :-( I have reverted adding constant fields. Too bad - the casts are back. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/ec344eef..fbf3c9a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=02-03 Stats: 29 lines in 1 file changed: 15 ins; 3 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 18:23:30 GMT, Daniel Fuchs wrote: >> 👍 I'd put `_MASK` in the name along with whatever name is used for the bits >> in the frame spec . > > @RogerRiggs Actually - I'm no longer sure that this will work: > > > jshell> char x = 0b1000_ > x ==> '耀' > > jshell> var y = ~x > y ==> -32769 > > jshell> char y = ~x > | Error: > | incompatible types: possible lossy conversion from int to char > | char y = ~x; > | ^^ So if x is a char, ~x seems to be an int :-( - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > 👍 I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . @RogerRiggs Actually - I'm no longer sure that this will work: jshell> char x = 0b1000_ x ==> '耀' jshell> var y = ~x y ==> -32769 jshell> char y = ~x | Error: | incompatible types: possible lossy conversion from int to char | char y = ~x; | ^^ - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Add _MASK suffix to char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/bbcf238b..ec344eef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=01-02 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > 👍 I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . Done - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Use char constant to avoid casts - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/6ef906e0..bbcf238b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=00-01 Stats: 29 lines in 1 file changed: 5 ins; 15 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:37:44 GMT, Roger Riggs wrote:
>> Yes - that's my understanding.
>
> These methods either set or clear a single bit in `firstChar`.
> The constant is an `int` so its complement is a 32 bit int.
>
> It could also be written as `~ (char) 0b100_000`. Then the 16 bit
> unsigned char would be complemented.
> Either way, the cast is needed to be explicit about the size of the constant.
>
> Another way to avoid the cast would be to define the bit positions as:
>
> `private static final char FIN_BIT = 0b1000_;
> ... etc.
> `
> Then the code in the methods would not need the cast.
>
>
> if (value) {
> firstChar |= FIN_BIT;
> } else {
> firstChar &= ~FIN_BIT;
> }
Ah! Good point. Maybe I should use a constant and get rid of the cast.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:52:07 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java > line 739: > >> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >> append >> 738: bufferLen += (int) len; >> 739: pos++; > > Would it be cleaner to do the cast in the codeLengthOf method, so it returns > an int? I believe the method returns an "unsigned int" - having the method return an int would then potentially cause `bufferLen + len <= 64` to yield true when it shouldn't. Hopefully @pavelrappo will comment. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:55:16 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java > line 222: > >> 220: // compiler will emit a warning if not cast >> 221: firstChar &= (char) ~0b1000_; >> 222: } > > Am I right in believing that there was an implicit cast to char here before > and the only change is to make it explicit? ie. there is no actual behavior > change? Yes - that's my understanding. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http @pavelrappo I would appreciate if you could review these changes - PR: https://git.openjdk.java.net/jdk/pull/8656
RFR: 8286386: Address possibly lossy conversions in java.net.http
In relation to [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find here a patch that addresses possibly lossy conversions in java.net.http - Commit messages: - Fix comments - 8286386: Address possibly lossy conversions in java.net.http Changes: https://git.openjdk.java.net/jdk/pull/8656/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8656&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286386 Stats: 27 lines in 3 files changed: 15 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Wed, 11 May 2022 11:34:33 GMT, Michael McMahon wrote: >> Hello Michael, >> Most users will be using the `HttpClient.newBuilder()` to create the >> builder, so this note about `UnsupportedOperationException` can be >> confusing. However, for implementations (libraries?) which provide their >> own implementation of the `java.net.http.HttpClient.Builder`, I think, this >> note would be relevant. This approach is similar to what we already have on >> `java.net.http.HttpClient.newWebSocketBuilder()` method. > > Sure, I just think when most developers read "The default implementation of > this method throws UOE" they will think they can't use it without > implementing something themselves. Library developers will figure it out > anyway. This is the standard wording that has been used throughout the JDK to document the behavior of default methods on interfaces. Unless we receive different feedback during the CSR review I'd suggest to leave it that way. The second sentence makes it clear that our concrete implementations override that default behavior. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Tue, 10 May 2022 13:51:37 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix javadoc link in test LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 13:36:18 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Daniel's review suggestion - add a test to verify the behaviour of the
>> localAddress() default method implementation on HttpClient.Builder
>> - Daniel's review suggestion - remove reference to "Internet Protocol" in
>> javadoc
>
> test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283:
>
>> 281: * Tests that the default method implementation of
>> 282: * {@link
>> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws
>> 283: * an {@link UnsupportedOperationException}
>
> Same remark here
Otherwise things look good to me - you should update the CSR if it needs to be
updated.
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 12:37:47 GMT, Jaikiran Pai wrote:
>> This change proposes to implement the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>>
>> The change introduces a new API to allow applications to build a
>> `java.net.http.HTTPClient` configured with a specific local address that
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Daniel's review suggestion - add a test to verify the behaviour of the
> localAddress() default method implementation on HttpClient.Builder
> - Daniel's review suggestion - remove reference to "Internet Protocol" in
> javadoc
test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 268:
> 266: /**
> 267: * Tests the {@link
> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
> 268: * behaviour when that method is called on a builder returned by
> {@link HttpClient#newBuilder()}
/**
* Tests the {@link
HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
```
This `{@link` looks broken - the `HttpClient,` prefix probably need to be
removed?
test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283:
> 281: * Tests that the default method implementation of
> 282: * {@link
> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws
> 283: * an {@link UnsupportedOperationException}
Same remark here
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v17]
On Mon, 9 May 2022 07:52:29 GMT, Jaikiran Pai wrote:
>> This change proposes to implement the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>>
>> The change introduces a new API to allow applications to build a
>> `java.net.http.HTTPClient` configured with a specific local address that
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 37 commits:
>
> - Merge latest from master branch
> - add a @build to force jtreg to show consistent test results and add the
> relevant permissions for security manager testing
> - Change the new API to accept an InetAddress instead of an
> InetSocketAddress, after inputs from Michael, Daniel and others
> - Merge latest from master
> - Implement HttpServerAdapters in test as suggested by Daniel
> - fix check when security manager is enabled
> - Add a unit test for the new HttpClient.Builder.localAddress method
> - Implement Daniel's suggestion - only support InetSocketAddress with port 0
> - Merge latest from master branch
> - Merge latest from master branch
> - ... and 27 more:
> https://git.openjdk.java.net/jdk/compare/b490a58e...d4a19dea
Changes requested by dfuchs (Reviewer).
src/java.net.http/share/classes/java/net/http/HttpClient.java line 398:
> 396: *
> 397: * @implSpec If the {@link #localAddress(InetAddress) local
> address} is a non-null
> 398: * Internet Protocol address and a security manager is
> installed, then
Here and in the @throws bellow we could now remove mention of `Internet
Protocol`.
For instance, here we could say
... is a non-null address and a security ...
src/java.net.http/share/classes/java/net/http/HttpClient.java line 411:
> 409: * the {@link #localAddress(InetAddress) local address}
> is an
> 410: * Internet Protocol address and the
> 411: * security manager's {@link SecurityManager#checkListen
> checkListen}
```
... has been installed and the security manager's {@link
SecurityManager#checkListen checkListen} ...
test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 274:
> 272: // setting null should work fine
> 273: builder.localAddress(null);
> 274: builder.localAddress(InetAddress.getLoopbackAddress());
We probably also need a MockHttpClientBuilder to test the behaviour of the
default implementation (check that it throws UOE).
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Integrated: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources
On Fri, 6 May 2022 11:04:29 GMT, Daniel Fuchs wrote: > Hi, > > Please find here a simple test fix that re-architecture ShortResponseBody for > better resource usage. > The test is split to test GET and POST separately and avoids testing GET > twice. This pull request has now been integrated. Changeset: f1433861 Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/f143386109bce2a2e7241f685e2df26849a0ad48 Stats: 501 lines in 5 files changed: 315 ins; 172 del; 14 mod 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources Reviewed-by: michaelm - PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources [v2]
On Mon, 9 May 2022 11:57:30 GMT, Michael McMahon wrote: >> Daniel Fuchs has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'master' into ShortResponseBody >> - 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should >> use less resources > > test/jdk/java/net/httpclient/ShortResponseBodyPost.java line 31: > >> 29: * @library /test/lib >> 30: * @build jdk.test.lib.net.SimpleSSLContext ShortResponseBody >> ShortResponseBodyGet >> 31: * @run testng/othervm > > Seems like the build directive should refer to ShortResponseBodyPost instead > of ShortResponseBodyGet Oh! You're right. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources [v3]
> Hi, > > Please find here a simple test fix that re-architecture ShortResponseBody for > better resource usage. > The test is split to test GET and POST separately and avoids testing GET > twice. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Fixed @build - Changes: - all: https://git.openjdk.java.net/jdk/pull/8569/files - new: https://git.openjdk.java.net/jdk/pull/8569/files/11959ee8..17cc353a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8569&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8569&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8569.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8569/head:pull/8569 PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources [v2]
> Hi, > > Please find here a simple test fix that re-architecture ShortResponseBody for > better resource usage. > The test is split to test GET and POST separately and avoids testing GET > twice. Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into ShortResponseBody - 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources - Changes: - all: https://git.openjdk.java.net/jdk/pull/8569/files - new: https://git.openjdk.java.net/jdk/pull/8569/files/b212c007..11959ee8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8569&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8569&range=00-01 Stats: 109734 lines in 1343 files changed: 97735 ins; 5210 del; 6789 mod Patch: https://git.openjdk.java.net/jdk/pull/8569.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8569/head:pull/8569 PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v3]
> Hi, please find here a patch that solves a rare intermittent test failure > observed in the test `java/net/httpclient/ExecutorShutdown.java` > > A race condition coupled with some too eager synchronization was causing a > deadlock between an Http2Connection close, a thread trying to shutdown the > HttpClient due to a RejectedTaskException, and the SelectorManager thread > trying to exit. > > The fix comprises several cleanup - in particular: > > - `Http2Connection`: no need to try to send a `GOAWAY` frame if the > underlying TCP connection is already closed > - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will > request more data from upstream if the sequential scheduler that is supposed > to handle that data once it arrives is already closed > - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an > exception is raised before `onSubscribe()` has been called > - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks > when not necessary > - `ReferenceTracker`: better thread dumps in case where the selector is still > alive at the end of the test (remove the limit that limited the stack traces > to 8 element max by no longer relying on `ThreadInfo::toString`) Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into ExecutorShutdown-intermittent-8286194 - Added a comment to ReferenceTracker.java as suggested in the review - 8286194: ExecutorShutdown test fails intermittently - Changes: - all: https://git.openjdk.java.net/jdk/pull/8562/files - new: https://git.openjdk.java.net/jdk/pull/8562/files/ce8ad93d..12bf40b0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8562&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8562&range=01-02 Stats: 102369 lines in 1228 files changed: 93484 ins; 3845 del; 5040 mod Patch: https://git.openjdk.java.net/jdk/pull/8562.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8562/head:pull/8562 PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v7]
On Thu, 5 May 2022 12:55:57 GMT, Michael Felt wrote:
>> with IP "0.0.0.0"
>>
>> - it either does nothing and ping fails, or, in some virtual environments
>> is treated as the default route address.
>> - IPv6 support for ::1 is available since 1977; however, ::0 is not accepted
>> as a vaild psuedo IPv6 address. '::1' must be used instead.
>>
>> ping: bind: The socket name is not available on this system.
>> ping: bind: The socket name is not available on this system.
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.037 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.045 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0/0/0 ms
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.052 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.047 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>>
>>
>> A long commit message.
>>
>> This came to light because some systems failed with IPv4 (those that passed
>> replaced 0.0.0.0 with the default router. but most just fail - not
>> substituting
>> 0.0.0.0 with 127.0.0.1. However, InetAddress.getByName("") returns 127.0.0.1
>> which compares well with other platform's behavior.
>
> Michael Felt has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changes per review
The last version looks good to me too. I agree that using `@requires` is a
cleaner way to skip or select the test. Thanks for making this change!
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v6]
On Fri, 6 May 2022 10:39:38 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8283544: Improved logging, drain input stream
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java line
> 311:
>
>> 309: streaming = true;
>> 310: systemHeadersBuilder.setHeader("Transfer-encoding",
>> "chunked");
>> 311: }
>
> With that - and if I'm not mistaken, lines 294-299 can be removed now.
hmmm... some parts seem to be missing now. I believe what we need is:
// Absence of a requestPublisher indicates a request with no body
// (GET, HEAD, DELETE), in which case we don't explicitly set any
// Content-Length header
if (requestPublisher != null) {
contentLength = requestPublisher.contentLength();
if (contentLength == 0) {
systemHeadersBuilder.setHeader("Content-Length", "0");
} else if (contentLength > 0) {
systemHeadersBuilder.setHeader("Content-Length",
Long.toString(contentLength));
streaming = false;
} else {
streaming = true;
systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8017
RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources
Hi, Please find here a simple test fix that re-architecture ShortResponseBody for better resource usage. The test is split to test GET and POST separately and avoids testing GET twice. - Commit messages: - 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources Changes: https://git.openjdk.java.net/jdk/pull/8569/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8569&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286293 Stats: 501 lines in 5 files changed: 315 ins; 172 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8569.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8569/head:pull/8569 PR: https://git.openjdk.java.net/jdk/pull/8569
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
Changes requested by dfuchs (Reviewer).
test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 41:
> 39: final InputStream is;
> 40: final int parentStream; // not the pushed streamid
> 41: private final List continuations;
Looks much better - thanks. If you changed that to `List` instead
it would allow you to add a test case where the server would send a regular
HeaderFrame on the pushPromiseStream before sending the continuation on the
original stream. You'd just have to put a HeaderFrame in that list before the
ContinuationFrame. That would reproduce the failure we had before your fix, and
that would allow you to verify that the bug that got the client hanging has
been fixed on the client side too.
test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 54:
> 52: this.continuations = null;
> 53: }
> 54:
This constuctor should call the other consdtructor below:
public OutgoingPushPromise(int parentStream,
URI uri,
HttpHeaders headers,
InputStream is) {
this(parentStream, uri, headers, is, List.of());
}
-
PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v6]
On Fri, 6 May 2022 10:33:31 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: Improved logging, drain input stream
Changes requested by dfuchs (Reviewer).
src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java line
311:
> 309: streaming = true;
> 310: systemHeadersBuilder.setHeader("Transfer-encoding",
> "chunked");
> 311: }
With that - and if I'm not mistaken, lines 294-299 can be removed now.
test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 105:
> 103: .uri(URI.create(testContentLengthURI + NO_BODY_PATH))
> 104: .build();
> 105: HttpClient hc = HttpClient.newHttpClient();
It would be better to reuse the same client for all tests. Also better to
configure it with NO_PROXY.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v2]
On Fri, 6 May 2022 05:16:15 GMT, Jaikiran Pai wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added a comment to ReferenceTracker.java as suggested in the review
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/SubscriberWrapper.java
> line 347:
>
>> 345:
>> 346: void upstreamWindowUpdate() {
>> 347: if (pushScheduler.isStopped()) return;
>
> A similar question here. It looks to me that the contract with a
> `SequentialScheduler` is that the runnable task itself (or someone with
> access to the scheduler), is responsible for invoking the `runOrSchedule`
> method so that a next invocation of the task happens. When such a scheduler
> instance is already stopped, then the call to `runOrSchedule` is kind of a
> noop, with no next execution of the task taking place. In cases like here,
> where the task detects that the scheduler has already stopped, do you think
> the tasks should do some cleanup work like clearing up the `outputQ` of
> `ByteBuffers`?
>
> The question really isn't directly related to the PR, but I have been
> reviewing some unrelated test failures where a large number of HttpClient
> instances get created, so in that context, I was wondering if there's
> anything we could do to help reduce any potential memory usage.
What happens here is a bit more subtle: if we don't return, the code is going
to request more data from upstream, even though that data won't be processed,
and that will fill up the queue, and potentially cause more data to be wrapped
by the SSLEngine and sent down stream. I have observed that some activity was
still ongoing in the SSLTube/SSLFlowDelegate after the underlying TCP channel
had been closed, when there's no chance that this data will be ever sent, and
the logs I was observing showed that returning at this point was effectively
stopping that useless activity early. The fact that the channel is
bidirectional and that the implementation is fully asynchronous means that
several tasks may be executing in background threads at the time the TCP
connection is closed. If the effect of these task is to simply request, process
and submit data that will be later ignored (or cause more exceptions to be
relayed) - then we should stop them early.
-
PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v2]
> Hi, please find here a patch that solves a rare intermittent test failure > observed in the test `java/net/httpclient/ExecutorShutdown.java` > > A race condition coupled with some too eager synchronization was causing a > deadlock between an Http2Connection close, a thread trying to shutdown the > HttpClient due to a RejectedTaskException, and the SelectorManager thread > trying to exit. > > The fix comprises several cleanup - in particular: > > - `Http2Connection`: no need to try to send a `GOAWAY` frame if the > underlying TCP connection is already closed > - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will > request more data from upstream if the sequential scheduler that is supposed > to handle that data once it arrives is already closed > - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an > exception is raised before `onSubscribe()` has been called > - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks > when not necessary > - `ReferenceTracker`: better thread dumps in case where the selector is still > alive at the end of the test (remove the limit that limited the stack traces > to 8 element max by no longer relying on `ThreadInfo::toString`) Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Added a comment to ReferenceTracker.java as suggested in the review - Changes: - all: https://git.openjdk.java.net/jdk/pull/8562/files - new: https://git.openjdk.java.net/jdk/pull/8562/files/0a674cef..ce8ad93d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8562&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8562&range=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8562.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8562/head:pull/8562 PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286194: ExecutorShutdown test fails intermittently
On Fri, 6 May 2022 04:57:00 GMT, Jaikiran Pai wrote:
>> Hi, please find here a patch that solves a rare intermittent test failure
>> observed in the test `java/net/httpclient/ExecutorShutdown.java`
>>
>> A race condition coupled with some too eager synchronization was causing a
>> deadlock between an Http2Connection close, a thread trying to shutdown the
>> HttpClient due to a RejectedTaskException, and the SelectorManager thread
>> trying to exit.
>>
>> The fix comprises several cleanup - in particular:
>>
>> - `Http2Connection`: no need to try to send a `GOAWAY` frame if the
>> underlying TCP connection is already closed
>> - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will
>> request more data from upstream if the sequential scheduler that is supposed
>> to handle that data once it arrives is already closed
>> - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an
>> exception is raised before `onSubscribe()` has been called
>> - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks
>> when not necessary
>> - `ReferenceTracker`: better thread dumps in case where the selector is
>> still alive at the end of the test (remove the limit that limited the stack
>> traces to 8 element max by no longer relying on `ThreadInfo::toString`)
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/SSLFlowDelegate.java
> line 784:
>
>> 782:
>> 783: while (Utils.synchronizedRemaining(writeList) > 0 ||
>> hsTriggered() || needWrap()) {
>> 784: if (scheduler.isStopped()) return;
>
> If the `scheduler` is stopped then would this task instance be ever called
> again? If it won't be called again, then do you think we should perhaps drain
> the queued `writeList` to reduce any memory resource accumulation till the
> next GC cycle?
if the scheduler is closed the connection is being stopped and will be shortly
eligible for garbage collection, so I wouldn't bother with trying to clear the
queue.
> test/jdk/java/net/httpclient/ReferenceTracker.java line 95:
>
>> 93: }
>> 94:
>> 95: private static String toString(ThreadInfo info) {
>
> Should we perhaps add a comment to this method explaining why we are
> duplicating the implementation of `ThreadInfo#toString()` here?
>
> I can't find in commit logs or the documentation of the `ThreadInfo` class on
> why its `toString()` implementation just outputs only 8 stacktrace elements.
> Do you think we should remove that restriction or document it in a separate
> issue?
Good idea to add a comment. I have also wondered if we should add a new method
to ThreadInfo that would take a "maxdepth" integer. I don't know why the output
is limited to 8 element. Maybe we should log an enhancement request and let the
maintainers of ThreadInfo decide if they want to remove the limitation or
provide a new method, or do nothing.
-
PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286194: ExecutorShutdown test fails intermittently
On Fri, 6 May 2022 04:49:53 GMT, Jaikiran Pai wrote:
>> Hi, please find here a patch that solves a rare intermittent test failure
>> observed in the test `java/net/httpclient/ExecutorShutdown.java`
>>
>> A race condition coupled with some too eager synchronization was causing a
>> deadlock between an Http2Connection close, a thread trying to shutdown the
>> HttpClient due to a RejectedTaskException, and the SelectorManager thread
>> trying to exit.
>>
>> The fix comprises several cleanup - in particular:
>>
>> - `Http2Connection`: no need to try to send a `GOAWAY` frame if the
>> underlying TCP connection is already closed
>> - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will
>> request more data from upstream if the sequential scheduler that is supposed
>> to handle that data once it arrives is already closed
>> - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an
>> exception is raised before `onSubscribe()` has been called
>> - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks
>> when not necessary
>> - `ReferenceTracker`: better thread dumps in case where the selector is
>> still alive at the end of the test (remove the limit that limited the stack
>> traces to 8 element max by no longer relying on `ThreadInfo::toString`)
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 1039:
>
>> 1037: e.abort(selectorClosedException());
>> 1038: } else {
>> 1039: selector.wakeup();
>
> Hello Daniel, before this PR, except for the `wakeupSelector()` method on
> `SelectorManager`, all other methods which were operating on the `selector`
> field were `synchronized` on the `SelectorManager` instance. After this PR,
> that continues to be true except for this specific line where the operation
> on the `selector` field is outside of a `synchronized` block. Would that be
> OK?
And this is what (or at least a part of what) was causing the issue. We need to
add the event to the `registrations` list within a synchronized block because
that's a plain ArrayList whose access is controlled by synchronizing on `this`.
However the event should not be invoked within the synchronized and block, and
if you look at the calling method (eventUpdated) you will see that this is what
we are doing there too (raising the event without synchronizing).
-
PR: https://git.openjdk.java.net/jdk/pull/8562
RFR: 8286194: ExecutorShutdown test fails intermittently
Hi, please find here a patch that solves a rare intermittent test failure observed in the test `java/net/httpclient/ExecutorShutdown.java` A race condition coupled with some too eager synchronization was causing a deadlock between an Http2Connection close, a thread trying to shutdown the HttpClient due to a RejectedTaskException, and the SelectorManager thread trying to exit. The fix comprises several cleanup - in particular: - `Http2Connection`: no need to try to send a `GOAWAY` frame if the underlying TCP connection is already closed - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will request more data from upstream if the sequential scheduler that is supposed to handle that data once it arrives is already closed - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an exception is raised before `onSubscribe()` has been called - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks when not necessary - `ReferenceTracker`: better thread dumps in case where the selector is still alive at the end of the test (remove the limit that limited the stack traces to 8 element max by no longer relying on `ThreadInfo::toString`) - Commit messages: - 8286194: ExecutorShutdown test fails intermittently Changes: https://git.openjdk.java.net/jdk/pull/8562/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8562&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286194 Stats: 135 lines in 7 files changed: 107 ins; 3 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/8562.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8562/head:pull/8562 PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 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.
Changes requested by dfuchs (Reviewer).
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()`
test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java line
950:
> 948: writeFrame(pp);
> 949: if (pp.getFlags() != HeadersFrame.END_HEADERS &&
> op.hasContinuations()) {
> 950: LinkedList continuations = new
> LinkedList<>(op.getContinuations());
That copy seems unneeded?
test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 39:
> 37: final InputStream is;
> 38: final int parentStream; // not the pushed streamid
> 39: private LinkedList continuations;
Make this `final` and instantiate it in the constructor instead.
Could use `List` here - or even `List` if you
want to simulate the bug we had before - e.g - you could add a `HeaderFrame`
instead of a `ContinuationFrame`, and verify the client no longer hangs and
rightfully closes the connection...
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)
test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 60:
> 58: public boolean hasContinuations() {
> 59: return !continuations.isEmpty();
> 60: }
That method is not needed. Plus it will throw if `continuations` is null.
test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 64:
> 62: public LinkedList getContinuations() {
> 63: return continuations;
> 64: }
Same remark here - you could use `List` instead of `LinkedList`.
-
PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 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.
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.
src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line
949:
> 947: decrementStreamsCount(streamid);
> 948: closeStream(streamid);
> 949: System.err.println("closeStream");
Either remove or use a debug logger for this new trace.
-
PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v4]
On Fri, 29 Apr 2022 15:06:28 GMT, Jaikiran Pai wrote: >> too much Python (no semi-colons) - great catch. >> >> Looking into how to verify proposed changes using jenkins (adoptium). When >> not in aqa-tests, more difficult (for me) too get it tested. (aka Better >> next time). > > @aixtools, if you enable GitHub Actions on your forked repo, it should then > automatically trigger a GitHub Actions job for your changes in your PR and > those results/run will be visible in this PR. The openjdk/jdk repo has been > configured to run `tier1` build/test on PR submission, so errors like this > are easily caught. Of course, if you build this locally, that's much quicker > to catch too. This is a test in tier2 though - so I don't expect it will be caught by github actions - PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v4]
On Fri, 29 Apr 2022 12:11:27 GMT, Michael Felt wrote: >> Good catch! > > too much Python (no semi-colons) - great catch. > > Looking into how to verify proposed changes using jenkins (adoptium). When > not in aqa-tests, more difficult (for me) too get it tested. (aka Better next > time). Still won't compile ;-) - osname should also be declared. You could do that with adding `var` before `osname` at line 48. Could you please also run the test and verify that it compiles? - PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v4]
On Thu, 28 Apr 2022 14:56:25 GMT, Michael McMahon wrote:
>> Michael Felt has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjusted and moved comments per review
>
> test/jdk/java/net/Inet4Address/PingThis.java line 49:
>
>> 47: public static void main(String args[]) throws Exception {
>> 48: osname = System.getProperty("os.name")
>> 49: /*
>
> Doesn't look like the line above will compile.
Good catch!
-
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v2]
On Wed, 27 Apr 2022 14:27:01 GMT, Michael McMahon wrote: >> src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c line 209: >> >>> 207: return JNI_FALSE; >>> 208: } >>> 209: fd = socket(AF_INET6, SOCK_DGRAM, 0); >> >> So if IPv6 is not supported on the machine, won't that result on reporting >> that IP don't fragment is unsupported? Same question for line 201, but for >> IPv6 only machines? > > I'm not sure you can disable IPv6 completely on mac OS. You can disable it on > a per-interface basis. Even then it leaves a link local address available and > that code succeeds. > > I was originally concerned that running with -Djava.net.preferIPv4Stack=true > might cause problems, but even in that case, the check succeeds because it > ignores that settting. OK - if you're confident that this will work. - PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v3]
On Wed, 27 Apr 2022 14:57:34 GMT, Michael McMahon wrote: >> Hi, >> >> Can I get the following fix reviewed please? JDK-8284890 was pushed >> yesterday and is causing failures on older versions of macOS that do not >> support the option. The fix here is to check at initialization time whether >> it is supported before adding it to the list of supported options. The error >> causes the new test and two existing ones to fail. I will remove the two >> tests from the problem list separately. >> >> Thanks, >> Michael. > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test update Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v4]
On Wed, 27 Apr 2022 15:27:45 GMT, Michael Felt wrote:
>> with IP "0.0.0.0"
>>
>> - it either does nothing and ping fails, or, in some virtual environments
>> is treated as the default route address.
>> - IPv6 support for ::1 is available since 1977; however, ::0 is not accepted
>> as a vaild psuedo IPv6 address. '::1' must be used instead.
>>
>> ping: bind: The socket name is not available on this system.
>> ping: bind: The socket name is not available on this system.
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.037 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.045 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0/0/0 ms
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.052 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.047 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>>
>>
>> A long commit message.
>>
>> This came to light because some systems failed with IPv4 (those that passed
>> replaced 0.0.0.0 with the default router. but most just fail - not
>> substituting
>> 0.0.0.0 with 127.0.0.1. However, InetAddress.getByName("") returns 127.0.0.1
>> which compares well with other platform's behavior.
>
> Michael Felt has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adjusted and moved comments per review
Marked as reviewed by dfuchs (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v2]
On Wed, 27 Apr 2022 12:10:13 GMT, Michael McMahon wrote:
>> Hi,
>>
>> Can I get the following fix reviewed please? JDK-8284890 was pushed
>> yesterday and is causing failures on older versions of macOS that do not
>> support the option. The fix here is to check at initialization time whether
>> it is supported before adding it to the list of supported options. The error
>> causes the new test and two existing ones to fail. I will remove the two
>> tests from the problem list separately.
>>
>> Thanks,
>> Michael.
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> updated test
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c line 209:
> 207: return JNI_FALSE;
> 208: }
> 209: fd = socket(AF_INET6, SOCK_DGRAM, 0);
So if IPv6 is not supported on the machine, won't that result on reporting that
IP don't fragment is unsupported? Same question for line 201, but for IPv6 only
machines?
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
> 42:
> 43: public static void main(String[] args) throws IOException {
> 44: isMacos = System.getProperty("os.name").equals("Mac OS X");
I believe there's a test library class that does that. I never remember what
the os.name is supposed to look like.
-
PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
On Wed, 27 Apr 2022 09:43:52 GMT, Michael McMahon wrote:
> Hi,
>
> Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday
> and is causing failures on older versions of macOS that do not support the
> option. The fix here is to check at initialization time whether it is
> supported before adding it to the list of supported options. The error causes
> the new test and two existing ones to fail. I will remove the two tests from
> the problem list separately.
>
> Thanks,
> Michael.
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 62:
> 60:
> 61: if (!c1.supportedOptions().contains(IP_DONTFRAGMENT)) {
> 62: return;
Should we assert that this should only happens on mac here? Same remark for the
two other places below...
-
PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 12:41:38 GMT, Сергей Цыпанов wrote: > Btw, I see that `java.net.URI` duplicates some methods from > `sun.net.www.ParseUtil`. Should we consolidate them e.g. in a way of reusing > functionality of `sun.net.www.ParseUtil` in `java.net.URI`? I don't think the ratio benefit/risk is high enough. - PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 12:39:50 GMT, Сергей Цыпанов wrote: >> src/java.base/share/classes/java/net/URI.java line 1921: >> >>> 1919: return sn - tn; >>> 1920: int val = 0; >>> 1921: int n = Math.min(sn, tn); >> >> Can we drop this change? I wouldn't like `java.net.URI` to gratuitously >> trigger the loading of the Math class. >> For the rest of the proposed changes, I will need to study them carefully >> and that may take some time. >> Thanks! > > @dfuch I've looked into class loading order and I see this: > > ... > [0.179s][info][class,init] 38 Initializing > 'java/util/ImmutableCollections$MapN'(no method) (0x00080013d4b8) > [0.180s][info][class,init] 39 Initializing 'java/lang/StringLatin1' > (0x000800021cb8) > [0.180s][info][class,init] 40 Initializing 'java/lang/Math' > (0x000800022828) > [0.180s][info][class,init] 41 Initializing 'java/lang/Number'(no method) > (0x000800030080) > [0.180s][info][class,init] 42 Initializing 'java/lang/Float' > (0x00080002fe10) > ... > [0.234s][info][class,init] 194 Initializing > 'java/lang/module/ModuleDescriptor$Requires$Modifier' (0x0008001def98) > [0.234s][info][class,init] 195 Initializing > 'java/lang/module/ModuleDescriptor$Provides'(no method) (0x0008001dd800) > [0.235s][info][class,init] 196 Initializing 'java/net/URI' > (0x000800142ff0) > [0.236s][info][class,init] 197 Initializing 'java/net/URI$1'(no method) > (0x0008001f4368) > [0.236s][info][class,init] 198 Initializing > 'jdk/internal/module/SystemModuleFinders$2'(no method) (0x0008001d4790) > [0.236s][info][class,init] 199 Initializing > 'jdk/internal/module/SystemModuleFinders$3'(no method) (0x0008001d50b0) > > I.e. `Math` is loaded far before `URI`. Am I missing something? OK. - PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 07:02:55 GMT, Сергей Цыпанов wrote: > - use `String.equalsIgnoreCase()` instead of hand-written code relying on > `String.charAt()` > - use `String.compareToIgnoreCase()` instead of hand-written code relying on > `String.charAt()` > - drop branches that are never executed > - drop unused argument from `URI.resolvePath()` > - simplify String-related operations src/java.base/share/classes/java/net/URI.java line 1921: > 1919: return sn - tn; > 1920: int val = 0; > 1921: int n = Math.min(sn, tn); Can we drop this change? I wouldn't like `java.net.URI` to gratuitously trigger the loading of the Math class. For the rest of the proposed changes, I will need to study them carefully and that may take some time. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) > @dfuch I have only updated files in `src`, so if the incorrect spelling is > tested, that test will now fail. I'm unfortunately not well versed in what > tests cover serviceability code. Can you suggest a suitable set of tests to > run? Folks from serviceability-dev will be more able to answer that - but I would suggest running tier2-tier3 as a precaution. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. LGTM. I spotted one fix in a exception message. I don't expect that there will be tests depending on that, but it might still be a good idea to run the serviceability tests to double check. Although I guess the test would have had the same typo and would have been fixed too were it the case :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8284890: Support for Do not fragment IP socket options [v8]
On Wed, 20 Apr 2022 14:30:21 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following PR review please? It adds a new JDK specific >> extended socket option >> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 >> and IPv6 >> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF >> (Dont Fragment) bit >> in the IP header. There is no equivalent in the IPv6 packet header as >> fragmentation is implemented >> exclusively by the sending and receiving nodes. >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test update Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8245
Integrated: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task
On Mon, 24 Jan 2022 14:25:09 GMT, Daniel Fuchs wrote: > These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. This pull request has now been integrated. Changeset: 5291ec8d Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/5291ec8d56b0e89aa96c3d53d9dcf093480cf48f Stats: 2001 lines in 24 files changed: 1733 ins; 139 del; 129 mod 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task Reviewed-by: jpai, michaelm - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v9]
On Sat, 16 Apr 2022 11:06:21 GMT, Daniel Fuchs wrote: >> These changes make sure that pending requests are terminated if the selector >> manager thread exits due to exceptions. >> This includes: >>1. completing CompletableFutures that were returned to the caller code >>2. cancelling requests that are in flight >>3. calling onError on BodySubscribers that may not have been completed >> Note that step 3 is necessary as certain CompletableFutures, such as those >> returned by `BodySubscribers.ofInputStream`, complete immediately, the >> operation being eventually completed when the last bite of the response is >> read. Completing a completable future that is already completed has no >> effect, this case is handled by completing the BodySubscriber too. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed accepted exception logging in tests Thanks Michael! - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Fri, 8 Apr 2022 10:19:08 GMT, Jaikiran Pai wrote:
>> Hello Conor,
>>
>> I had a look at this latest update to the `Http1Request`. The github diff
>> isn't easy to understand/explain in this case, so I'll paste here the latest
>> code contained in this PR, from that method. It looks like:
>>
>>
>> if (requestPublisher == null) {
>> // Not a user request, or maybe a method, e.g. GET, with no body.
>> contentLength = 0;
>> } else {
>> contentLength = requestPublisher.contentLength();
>> }
>>
>> // GET, HEAD and DELETE with no request body should not set the
>> Content-Length header
>> if (requestPublisher != null) {
>> if (contentLength == 0) {
>> systemHeadersBuilder.setHeader("Content-Length", "0");
>> } else if (contentLength > 0) {
>> systemHeadersBuilder.setHeader("Content-Length",
>> Long.toString(contentLength));
>> streaming = false;
>> } else {
>> streaming = true;
>> systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
>> }
>> }
>>
>> I think we don't need the additional/new ` if (requestPublisher != null)`
>> block and can instead move the contents of this `if` block into the
>> immediately preceding `else` block. Thinking a bit more, I think this entire
>> above code can be reduced to just:
>>
>>
>> // absence of a requestPublisher indicates a request with no body, in which
>> // case we don't explicitly set any Content-Length header
>> if (requestPublisher != null) {
>> var contentLength = requestPublisher.contentLength();
>> if (contentLength == 0) {
>> systemHeadersBuilder.setHeader("Content-Length", "0");
>> } else if (contentLength > 0) {
>> systemHeadersBuilder.setHeader("Content-Length",
>> Long.toString(contentLength));
>> streaming = false;
>> } else {
>> streaming = true;
>> systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
>> }
>> }
>>
>> and the previous if/else block completely deleted. The absence of a
>> `requestPublisher` would mean a request with no body.
>>
>> Additionally, I noticed that the `HttpRequest.Builder` does this for `HEAD`
>> method:
>>
>>
>> /**
>> * Sets the request method of this builder to HEAD.
>> *
>> * @implSpec The default implementation is expected to have the same
>> behaviour as:
>> * {@code return method("HEAD", BodyPublishers.noBody());}
>> *
>> * @return this builder
>> * @since 18
>> */
>> default Builder HEAD() {
>> return method("HEAD", BodyPublishers.noBody());
>> }
>>
>> This is unlike other methods, for example `DELETE()` where the body
>> publisher itself is `null`. In the case of `HEAD` the body publisher is
>> present but it still represents that there's no body to that request. Should
>> we perhaps detect even this specific case (i.e. `instanceof
>> RequestPublishers.EmptyPublisher`) and skip setting the `Content-Length`
>> header. If we don't add this additional check, from what I see with this
>> updated code now, we will still end up explicitly setting `Content-Length`
>> to `0` when a `HEAD` request is generated using the
>> `HttpRequest.Builder.HEAD()` API, since the `EmptyPublisher` will return `0`
>> from its `contentLength()` implementation.
>
>> This is unlike other methods, for example DELETE() where the body publisher
>> itself is null. In the case of HEAD the body publisher is present but it
>> still represents that there's no body to that request.
>
> Please disregard this part of the comment. As you and Daniel rightly noted in
> a private conversation, the `HttpRequestBuilderImpl` overrides the `HEAD()`
> method to set `null` to the body publisher.
@jaikiran @c-cleary
// absence of a requestPublisher indicates a request with no body, in which
// case we don't explicitly set any Content-Length header
if (requestPublisher != null) {
var contentLength = requestPublisher.contentLength();
if (contentLength == 0) {
systemHeadersBuilder.setHeader("Content-Length", "0");
} else if (contentLength > 0) {
systemHeadersBuilder.setHeader("Content-Length",
Long.toString(contentLength));
streaming = false;
} else {
streaming = true;
systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
}
}
Sounds good to me.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Tue, 19 Apr 2022 16:44:35 GMT, Daniel Fuchs wrote:
>> test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
>>
>>> 200: } else {
>>> 201: String responseBody = exchange.getRequestMethod() + "
>>> request contained an unexpected " +
>>> 202: "Content-length header.";
>>
>> Maybe the message could include the value of `Content-Length` that was
>> received.
>
> Also it's always better to drain the request input stream even if there is no
> bytes.
You could possibly do that in `handleResponse()`
-
PR: https://git.openjdk.java.net/jdk/pull/8017
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
test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
> 200: } else {
> 201: String responseBody = exchange.getRequestMethod() + "
> request contained an unexpected " +
> 202: "Content-length header.";
Maybe the message could include the value of `Content-Length` that was received.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Tue, 19 Apr 2022 16:43:12 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8283544: Updated URI creation
>
> test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
>
>> 200: } else {
>> 201: String responseBody = exchange.getRequestMethod() + "
>> request contained an unexpected " +
>> 202: "Content-length header.";
>
> Maybe the message could include the value of `Content-Length` that was
> received.
Also it's always better to drain the request input stream even if there is no
bytes.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 15:54:02 GMT, Michael McMahon wrote:
>> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
>>
>>> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
>>> INET6;
>>> 43: System.out.println("Family = " + fam);
>>> 44: testDatagramChannel(args, fam);
>>
>> Shouldn't there be a testcase for when DatagramChannel is opened using the
>> no arg factory method `DatagramChannel.open()`?
>
> I'm not sure there is value in testing all of these permutations.
> Distinguishing DatagramChannel and DatagramSocket probably made sense, but
> it's all the same implementation under the hood.
1. `DatagramChannel.open()` => opens a dual socket unless
`-Djava.net.preferIPv4Stack=true`, in which case it should be equivalent to
`DatagramChannel.open(StandardProtocolFamily.INET)`
2. `DatagramChannel.open(StandardProtocolFamily.INET)` => opens an IPv4 socket
3. `DatagramChannel.open(StandardProtocolFamily.INET6)` => opens an IPv6 socket
So I believe it makes sense to test the no-arg constructor since that's the
only way to open a dual socket.
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 14:47:01 GMT, Michael McMahon wrote:
>> Hi,
>>
>> Could I get the following PR review please? It adds a new JDK specific
>> extended socket option
>> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4
>> and IPv6
>> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF
>> (Dont Fragment) bit
>> in the IP header. There is no equivalent in the IPv6 packet header as
>> fragmentation is implemented
>> exclusively by the sending and receiving nodes.
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix whitespace
src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c line 112:
> 110: return optval;
> 111: }
> 112: handleError(env, rv, "get option IP_DONTFRAGMENT failed");
Is there some indentation issue here?
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
> INET6;
> 43: System.out.println("Family = " + fam);
> 44: testDatagramChannel(args, fam);
Shouldn't there be a testcase for when DatagramChannel is opened using the no
arg factory method `DatagramChannel.open()`?
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 47:
> 45: try (DatagramSocket c = new DatagramSocket()) {
> 46: testDatagramSocket(c);
> 47: }
Can't you test `MulticastSocket` in exactly the same way? Why is there a
specific test method for `MulticastSocket`?
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v9]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Fixed accepted exception logging in tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/7196/files - new: https://git.openjdk.java.net/jdk/pull/7196/files/559c8523..b8a0ff29 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=07-08 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v8]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request incrementally with two additional commits since the last revision: - File forgotten in previous commit - Don't try to write to channel if selector is closed - Changes: - all: https://git.openjdk.java.net/jdk/pull/7196/files - new: https://git.openjdk.java.net/jdk/pull/7196/files/09899034..559c8523 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=06-07 Stats: 16 lines in 2 files changed: 16 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v7]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Improve diagnostics for tracking unreleased references - Changes: - all: https://git.openjdk.java.net/jdk/pull/7196/files - new: https://git.openjdk.java.net/jdk/pull/7196/files/4953167a..09899034 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=05-06 Stats: 25 lines in 1 file changed: 16 ins; 6 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v6]
On Fri, 15 Apr 2022 14:35:33 GMT, Daniel Fuchs wrote: >> These changes make sure that pending requests are terminated if the selector >> manager thread exits due to exceptions. >> This includes: >>1. completing CompletableFutures that were returned to the caller code >>2. cancelling requests that are in flight >>3. calling onError on BodySubscribers that may not have been completed >> Note that step 3 is necessary as certain CompletableFutures, such as those >> returned by `BodySubscribers.ofInputStream`, complete immediately, the >> operation being eventually completed when the last bite of the response is >> read. Completing a completable future that is already completed has no >> effect, this case is handled by completing the BodySubscriber too. > > Daniel Fuchs has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed exception handling in tests > - Better exception reporting While retesting after the rebase I got some intermittent test failures, mostly in the new tests (and some others that were unrelated). I have updated this PR with a small change that makes the new tests more stable. See combined changes brought by [2ea851b](https://github.com/openjdk/jdk/pull/7196/commits/2ea851b267ab5fa81eda0a9c9cb2e6cc0315c568) and [2ea851b](https://github.com/openjdk/jdk/pull/7196/commits/2ea851b267ab5fa81eda0a9c9cb2e6cc0315c568) @Michael-Mc-Mahon could you review these new changes? - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v6]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request incrementally with two additional commits since the last revision: - Fixed exception handling in tests - Better exception reporting - Changes: - all: https://git.openjdk.java.net/jdk/pull/7196/files - new: https://git.openjdk.java.net/jdk/pull/7196/files/c86e3766..4953167a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=04-05 Stats: 202 lines in 6 files changed: 82 ins; 90 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8284890: Support for Do not fragment IP socket options
On Thu, 14 Apr 2022 16:04:22 GMT, Michael McMahon wrote:
> Hi,
>
> Could I get the following PR review please? It adds a new JDK specific
> extended socket option
> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4
> and IPv6
> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF
> (Dont Fragment) bit
> in the IP header. There is no equivalent in the IPv6 packet header as
> fragmentation is implemented
> exclusively by the sending and receiving nodes.
>
> Thanks,
> Michael
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 40:
> 38:
> 39: public class DontFragmentTest {
> 40:
Should we have a similar test for DatagramSocket / MulticastSocket /
DatagramChannel.socket() ?
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Integrated: 8284892: java/net/httpclient/http2/TLSConnection.java fails intermittently
On Thu, 14 Apr 2022 18:26:00 GMT, Daniel Fuchs wrote: > java/net/httpclient/http2/TLSConnection.java has been observed failing (even > though rarely) in test jobs. > > The issue is that the handler used on the the server sides maintains a > volatile `sslSession` field which it sets when receiving a request, so that > the client can check which SSLParameters were negotiated after receiving the > response. > However that field is set a little too late: after writing the request body > bytes. This means that sometimes the client is able to receive the last byte > before the server has updated the field, and can observe the previous value, > which was set by the previous request. Checking of SSL parameters on the > client side then usually fails, as it is looking at the wrong session. > > The proposed fix is very simple: just update the `sslSession` field a little > earlier - before writing the response. This pull request has now been integrated. Changeset: 1e22c70f Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/1e22c70ff2e010740cb22856a642dd4afa1017cc Stats: 4 lines in 1 file changed: 2 ins; 1 del; 1 mod 8284892: java/net/httpclient/http2/TLSConnection.java fails intermittently Reviewed-by: djelinski, jpai, michaelm - PR: https://git.openjdk.java.net/jdk/pull/8249
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v5]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' into executor-shutdown-8277969 - Merge branch 'master' into executor-shutdown-8277969 - Merge branch 'master' into executor-shutdown-8277969 - Merge - asserts that acquired == true - Incorporated review comments - Merge branch 'master' into executor-shutdown-8277969 - 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task - Changes: https://git.openjdk.java.net/jdk/pull/7196/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7196&range=04 Stats: 1975 lines in 23 files changed: 1715 ins; 139 del; 121 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8284892: java/net/httpclient/http2/TLSConnection.java fails intermittently [v2]
> java/net/httpclient/http2/TLSConnection.java has been observed failing (even > though rarely) in test jobs. > > The issue is that the handler used on the the server sides maintains a > volatile `sslSession` field which it sets when receiving a request, so that > the client can check which SSLParameters were negotiated after receiving the > response. > However that field is set a little too late: after writing the request body > bytes. This means that sometimes the client is able to receive the last byte > before the server has updated the field, and can observe the previous value, > which was set by the previous request. Checking of SSL parameters on the > client side then usually fails, as it is looking at the wrong session. > > The proposed fix is very simple: just update the `sslSession` field a little > earlier - before writing the response. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Copyright update - Changes: - all: https://git.openjdk.java.net/jdk/pull/8249/files - new: https://git.openjdk.java.net/jdk/pull/8249/files/91626f6d..6bdb7a62 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8249&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8249&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8249.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8249/head:pull/8249 PR: https://git.openjdk.java.net/jdk/pull/8249
