Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v7]
> 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
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7013/files
- new: https://git.openjdk.java.net/jdk/pull/7013/files/56c37474..afe59252
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7013&range=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7013&range=05-06
Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/7013.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7013/head:pull/7013
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v6]
On Tue, 3 May 2022 09:11:41 GMT, Michael McMahon wrote: >> Michael Felt has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use @requires jtreg tag rather than check in code > > test/jdk/java/net/Inet4Address/PingThis.java line 32: > >> 30: * @requires os.family != "windows" && os.family != "aix" >> 31: * @library /test/lib >> 32: * @summary InetAddress.isReachable is returning false > > '&&' double ampersand syntax does not work here. Should be a single ampersand > as suggested by Aleksei. Once a C programmer, hard to resist. Thx. :) - PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v6]
On Tue, 3 May 2022 11:13:37 GMT, Aleksei Efimov wrote: >> Michael Felt has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use @requires jtreg tag rather than check in code > > test/jdk/java/net/Inet4Address/PingThis.java line 33: > >> 31: * @library /test/lib >> 32: * @summary InetAddress.isReachable is returning false >> 33: * for InetAdress 0.0.0.0 and ::0 > > Typo: `InetAdress` -> `InetAddress` I did not actually change that line - but fixing :) > test/jdk/java/net/Inet4Address/PingThis.java line 37: > >> 35: * but does not respond as 127.0.0.1 (i.e., only responds >> 36: * when an external device reacts to 0.0.0.0) >> 37: */ > > This line should be removed to fix formatting of the comment section, ie > unbalanced `*/` Thx! - PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v2]
> **Issue** > A recent fix for > [JDK-8263031](https://bugs.openjdk.java.net/browse/JDK-8263031), which > addressed the httpclient being unable to properly process Http/2 PushPromise > frames followed by continuations caused intermittent failures of the test. > This was cause by two seperate problems. > > - Firstly, on the client side, `Http2Connection.processFrame` did not a check > for whether or not a Continuation was to be expected early on in the method > resulting in frames other than the expected Continuation Frame being > incorrectly processed. In Http/2, when a Continuation Frame is expected, no > other type of frame should be processed without a connection error of type > PROTOCOL_ERROR being thrown. > - Secondly, `Http2TestConnection.handlePush` had no guards around response > headers and data frames being sent before a continuation is sent which > resulted in the intermittent nature of this timeout. > > **Proposed Solution** > The test class `OutgoingPushPromise` was modified to contain a list of > Continuation Frames as required rather than the Continuations being scheduled > to send in the test itself. This prevents the situation of unexpected frames > being sent before a Continuation Frame was meant to arrive. > > In addition to the above, Http2Connection was modified to ensure that a > connection error of type PROTOCOL_ERROR is thrown if a frame other than a > Continuation arrives at the client unexpectedly. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8284585: New constructor & minor improvements - Changes: - all: https://git.openjdk.java.net/jdk/pull/8518/files - new: https://git.openjdk.java.net/jdk/pull/8518/files/cb6c189b..24e702a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8518&range=00-01 Stats: 48 lines in 4 files changed: 18 ins; 16 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8518/head:pull/8518 PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 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
LGTM
-
Marked as reviewed by michaelm (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7013
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: 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
Latest changes look good to me. Will check that the test runs/not runs with our
CI and will sponsor it once I have the results.
-
Marked as reviewed by aefimov (Committer).
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: 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`)
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?
-
PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 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`)
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?
-
PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 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`)
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 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`)
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?
-
PR: https://git.openjdk.java.net/jdk/pull/8562
