Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
> Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony Anthony Scarpino has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - review update - update some nits - PR ready - Initial - Changes: https://git.openjdk.java.net/jdk/pull/8462/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8462=01 Stats: 393 lines in 3 files changed: 291 ins; 20 del; 82 mod Patch: https://git.openjdk.java.net/jdk/pull/8462.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8462/head:pull/8462 PR: https://git.openjdk.java.net/jdk/pull/8462
Integrated: 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. This pull request has now been integrated. Changeset: 76caeed4 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/76caeed498d868c7923461fb481349c0a2cbd99d Stats: 303 lines in 136 files changed: 0 ins; 0 del; 303 mod 8285366: Fix typos in serviceability Reviewed-by: kevinw, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability [v2]
> 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. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert "invocable" - Changes: - all: https://git.openjdk.java.net/jdk/pull/8334/files - new: https://git.openjdk.java.net/jdk/pull/8334/files/4b26668d..ad36a59a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8334=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8334=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8334.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8334/head:pull/8334 PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 14:18:04 GMT, Roger Riggs wrote: >> Well spotted, I don't think that change was intentionally. > > Ouch; Will fix: > > I took Alan's earlier comment literally: > > "This one can also be changed to: > > to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);" @RogerRiggs This already got fixed by @jaikiran in [GH‑8693]. [GH‑8693]: https://github.com/openjdk/jdk/pull/8693 - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 05:54:15 GMT, Alan Bateman wrote: >> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: >> >>> 126: // timed poll interrupted so need to adjust timeout >>> 127: long adjust = System.nanoTime() - startTime; >>> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); >> >> This will now always assign a negative number to `to`. >> >> >> >> `=-` is not a compound assignment, it’s negation followed by a normal >> assignment. > > Well spotted, I don't think that change was intentionally. Ouch; Will fix: I took Alan's earlier comment literally: "This one can also be changed to: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);" - PR: https://git.openjdk.java.net/jdk/pull/8642
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 Sat, 7 May 2022 11:46:37 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`) > > 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 Change looks fine to me. We can revisit the ThreadInfo code later. - Marked as reviewed by michaelm (Reviewer). 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
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v3]
On Sat, 7 May 2022 11:46:37 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`) > > 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 Good to see the dead code removed from HttpClientImpl (cancel). 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? Okay, I see the same issue has been raised already. I agree we should request that enhancement. - PR: https://git.openjdk.java.net/jdk/pull/8562
