Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v7]
On Tue, 16 Jan 2024 09:11:01 GMT, Chris Hegarty wrote: >> Update LinkedTransferQueue add and put methods to not call overridable offer. > > Chris Hegarty 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 eight additional > commits since the last revision: > > - Merge branch 'master' into ltq_bug > - Merge branch 'master' into ltq_bug > - order of tags > - Merge branch 'master' into ltq_bug > - Update > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > >Co-authored-by: Andrey Turbanov > - timed offer > - add test > - Update LinkedTransferQueue add and put methods to not call overridable > offer I just integrated the fix into jdk 22, so we’re good there now. The final piece of the puzzle is jdk 21.0.2, which we’re too late to fix. We can add a release note, and fix it in 21.0.3. Any objections or alternative suggestions? - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1894710437
[jdk22] Integrated: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Tue, 16 Jan 2024 12:23:43 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [ee4d9aa4](https://github.com/openjdk/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 16 Jan 2024 and > was reviewed by Alan Bateman. > > Thanks! This pull request has now been integrated. Changeset: c1ea6daa Author:Chris Hegarty URL: https://git.openjdk.org/jdk22/commit/c1ea6daa5bd3ecee4fd3f8acaf91dfa48ec02f1b Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod 8323659: LinkedTransferQueue add and put methods call overridable offer Reviewed-by: alanb Backport-of: ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 - PR: https://git.openjdk.org/jdk22/pull/80
[jdk22] RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
Hi all, This pull request contains a backport of commit [ee4d9aa4](https://github.com/openjdk/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Chris Hegarty on 16 Jan 2024 and was reviewed by Alan Bateman. Thanks! - Commit messages: - Backport ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 Changes: https://git.openjdk.org/jdk22/pull/80/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=80=00 Issue: https://bugs.openjdk.org/browse/JDK-8323659 Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk22/pull/80.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/80/head:pull/80 PR: https://git.openjdk.org/jdk22/pull/80
Integrated: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 10:38:40 GMT, Chris Hegarty wrote: > Update LinkedTransferQueue add and put methods to not call overridable offer. This pull request has now been integrated. Changeset: ee4d9aa4 Author: Chris Hegarty URL: https://git.openjdk.org/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod 8323659: LinkedTransferQueue add and put methods call overridable offer Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v7]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty 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 eight additional commits since the last revision: - Merge branch 'master' into ltq_bug - Merge branch 'master' into ltq_bug - order of tags - Merge branch 'master' into ltq_bug - Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - timed offer - add test - Update LinkedTransferQueue add and put methods to not call overridable offer - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/3aa026fa..ddaab989 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=05-06 Stats: 125 lines in 18 files changed: 86 ins; 30 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Mon, 15 Jan 2024 09:49:53 GMT, Alan Bateman wrote: >> With my CSR hat on, JDK-8301341 should never have made the changes it did >> without going through a CSR request. We have been bitten by this kind of >> problem many times. Unless a public method is specified to utilise another >> public method, we should never implement one public method in terms of >> another (non-final one) due to the overriding problem. Backporting such a >> change to 21u is then potentially very damaging. > >> With my CSR hat on, JDK-8301341 should never have made the changes it did >> without going through a CSR request. We have been bitten by this kind of >> problem many times. Unless a public method is specified to utilise another >> public method, we should never implement one public method in terms of >> another (non-final one) due to the overriding problem. > > JDK-8301341 was a big update, a lot of refactoring to hollow out SQ and it > was just an oversight that we didn't spot the methods now use an overridable > method. It's something to always look out for in the collections area, just > missed here. Thanks for the reviews @AlanBateman and @DougLea - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892838717
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v6]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty 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 seven additional commits since the last revision: - Merge branch 'master' into ltq_bug - order of tags - Merge branch 'master' into ltq_bug - Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - timed offer - add test - Update LinkedTransferQueue add and put methods to not call overridable offer - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/72ad71fb..3aa026fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=04-05 Stats: 350 lines in 17 files changed: 297 ins; 37 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
On Mon, 15 Jan 2024 12:34:57 GMT, Doug Lea wrote: > Sorry for needlessly calling overridable versions in these two cases. I > should have caught that. No problem, easy to overlook. I assume you are ok with the changes? If so, could I please ask you to add your review. Otherwise, is there another way that we should proceed? - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892569945
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty 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 five additional commits since the last revision: - Merge branch 'master' into ltq_bug - Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - timed offer - add test - Update LinkedTransferQueue add and put methods to not call overridable offer - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/22f2857c..72ad71fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=03-04 Stats: 11818 lines in 198 files changed: 5468 ins; 5671 del; 679 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v4]
On Mon, 15 Jan 2024 09:49:39 GMT, Chris Hegarty wrote: >> Update LinkedTransferQueue add and put methods to not call overridable offer. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > > Co-authored-by: Andrey Turbanov It's unfortunate that we're only discovering this now, since 21.0.2 is scheduled to release tomorrow, Jan 16th, and we've not yet gotten this reviewed and merged into _master_ or _jdk22_ yet. We can decided how to proceed with the backports once this PR has agreed the approach and is reviewed. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892039902
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Mon, 15 Jan 2024 07:31:13 GMT, Andrey Turbanov wrote: >> Chris Hegarty has updated the pull request incrementally with one additional >> commit since the last revision: >> >> timed offer > > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > line 1162: > >> 1160: Objects.requireNonNull(e); >> 1161: xfer(e, -1L); >> 1162: return true;} > > Suggestion: > > return true; > } Thank you @turbanoff - accepted and committed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17393#discussion_r1452141389
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v4]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/1097a6bc..22f2857c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: timed offer - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/b11804d1..1097a6bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=01-02 Stats: 10 lines in 2 files changed: 8 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Fri, 12 Jan 2024 15:02:59 GMT, Chris Hegarty wrote:
>> Update LinkedTransferQueue add and put methods to not call overridable offer.
>
> Chris Hegarty has updated the pull request incrementally with one additional
> commit since the last revision:
>
> timed offer
src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java line
1160:
> 1158: */
> 1159: public boolean offer(E e, long timeout, TimeUnit unit) {
> 1160: Objects.requireNonNull(e);
While here, it makes sense to update the timed offer variant, since it is
affected in a similar way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17393#discussion_r1450573006
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v2]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: add test - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/206e2652..b11804d1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17393=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17393=00-01 Stats: 60 lines in 1 file changed: 60 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 10:38:40 GMT, Chris Hegarty wrote: > Update LinkedTransferQueue add and put methods to not call overridable. FTR - I agree that it's kinda annoying to be proposing this change and it is true that the consuming user code is making an assumption, but the impact of this is significant - leads to apparently vanishing tasks when Elasticsearch runs on JDK 22 EA. The proposed changes are extremely minimal. If @DougLea agrees, then I'll add a minimal test case and get the PR into a clean state. Additionally, we absolutely need to fix this in JDK 21.0.2 - since a patch release should not change behaviour (from 21.0.1), in this way. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1889097252 PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1889103827
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 11:22:13 GMT, Chris Hegarty wrote: >>> A process-related comment: this PR is against mainline, but the issue's Fix >>> Version/s is 22. This will create a bit of a mess if integrated. >> >> Thanks for the reminder Pavel. If accepted, then the change will be >> applicable to 23, 22, and 21. 0.2. I'll fix up and retarget the appropriate >> branches, repos, once we agree a way forward. > >> this PR is against mainline, but the issue's Fix Version/s is 22. > > I updated the fix version in JIRA, and followed the process as outlined in > https://mail.openjdk.org/pipermail/jdk-dev/2023-December/008560.html > @ChrisHegarty My 2¢—this change could impact existing classes which have > extended LTQ and knowingly, or unknowingly, presume that add and put delegate > to offer, which would (silently) break those classes when running on a newer > JDK (presuming this change gets merged). > > I think the change is in principle correct, with that said, I want to make > sure that the change is worth the potential compatibility risk (if any). Hi @viktorklang-ora , I agree with your comment regarding the potential impact, and the assumption of the implementation, however, you got it the wrong way round! ;-) The change I am proposing restores previous behaviour to pre-JDK 22. It is JDK 22 that has changed this. This is exactly why I am proposing the change - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1889016400
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 11:11:51 GMT, Chris Hegarty wrote: > this PR is against mainline, but the issue's Fix Version/s is 22. I updated the fix version in JIRA, and followed the process as outlined in https://mail.openjdk.org/pipermail/jdk-dev/2023-December/008560.html - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1888916748
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 11:07:17 GMT, Pavel Rappo wrote: > A process-related comment: this PR is against mainline, but the issue's Fix > Version/s is 22. This will create a bit of a mess if integrated. Thanks for the reminder Pavel. If accepted, then the change will be applicable to 23, 22, and 21. 0.2. I'll fix up and retarget the appropriate branches, repos, once we agree a way forward. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1888901110
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 11:03:02 GMT, Alan Bateman wrote: > This feels more like a hazard when extending to override behavior. Yeah, this is certainly a potential issue with any subclass-able class. I remember seeing similar before in other areas too. > I think it would be useful to provide a summary on what the override wants to > do, maybe there are better ways to customise by wrapping the LTQ rather than > subclassing. Yeah, and we can likely refactor our code to workaround this change in JDK 22. I added a summary and reproducer in the JIRA, which should help. I'll try to explain a little here. The crux of what the code is attempting to do is to scale the executor to max pool size by refusing tasks if there is the pool is less than the given max, then later intercepting rejections and putting the task on the queue. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-198399
RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
Update LinkedTransferQueue add and put methods to not call overridable. - Commit messages: - Update LinkedTransferQueue add and put methods to not call overridable offer Changes: https://git.openjdk.org/jdk/pull/17393/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17393=00 Issue: https://bugs.openjdk.org/browse/JDK-8323659 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Thu, 26 Oct 2023 09:17:25 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Allow unaligned array access LGTM - Marked as reviewed by chegar (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16360#pullrequestreview-1699260449
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 14:01:09 GMT, Maurizio Cimadamore
wrote:
>> This PR proposes removing the restriction that only heap `MemorySegment`
>> wrapping a `byte` array can be accessed by Vectors. Now any array type can
>> be used provided the element alignment constraints are respected.
>
> test/jdk/jdk/incubator/vector/AbstractVectorLoadStoreTest.java line 118:
>
>> 116: private static boolean canBeConverted(IntFunction
>> function, ValueLayout elementLayout) {
>> 117: // Create a sample to analyze
>> 118: MemorySegment s = function.apply(Long.BYTES);
>
> I believe that a good way to test this is the following:
> * each vector type operates as having a given element layout - for instance,
> you can imagine the layout for IntVector to be `JAVA_INT` and so forth
> * asking whether you load a segment into a vector is the same as asking
> whether you can access the segment, at offset 0L, with the layout associated
> with the vector (see above) - that is if MS::get throws, then vector load
> should also throw (and viceversa)
Vectors use and have in their docs, a layout with a byte alignment of 1. e.g.
from `IntVector`
ValueLayout.OfInt ELEMENT_LAYOUT = ValueLayout.JAVA_INT.withByteAlignment(1)
Can the `fromMemorySegment` not just behave similarly? I get that alignment is
preferable, but does it need to be enforced? If so, then maybe the
`ELEMENT_LAYOUT` and example in the javadoc needs to be updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1372042718
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: yes -Chris > On 25 Aug 2023, at 16:45, Joe Wang wrote: > > Vote: yes > > -Joe > >> On 8/25/23 8:23 AM, Roger Riggs wrote: >> I hereby nominate Lance Andersen to Membership in the Core Libraries Group >
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: yes -Chris > On 26 Aug 2023, at 08:13, Alan Bateman wrote: > > Vote: yes >
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: yes -Chris > On 26 Aug 2023, at 08:13, Alan Bateman wrote: > > Vote: yes
Re: CFV: New Core Libraries Group Member: Joe Wang
Vote: Yes. -Chris On 27/07/2023 19:36, Roger Riggs wrote: |I hereby nominate Joe Wang||to Membership in the Core Libraries Group Joe has been working in the core library team since 2012. He has been maintaining and improving the XML library APIs and implementations including ||DOM, SAX, StAX, Transform, Validation, XPath and Catalog. Recently he has been updating and improving XML Security. Votes are due by August 11th, 2023. Only current Members of the |||Core Libraries Group| [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote|
Re: CFV: New Core Libraries Group Member: Brent Christian
Vote: Yes. -Chris On 27/07/2023 19:36, Roger Riggs wrote: |I hereby nominate |Brent Christian|to Membership in the Core Libraries Group Brent has been working in the Core Library team at Oracle since 2012. He has made contributions to many areas of OpenJDK including java.lang strings, class loader, NIO Jar, and many more. Most recently he has been working on steps to deprecate and phase out finalization.| ||Previously he worked on OpenJDK AWT, Swing, and JavaFX before joining the Core Libraries group.| Votes are due by August 11th, 2023. Only current Members of the |||Core Libraries Group| [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote|
Re: CFV: New Core Libraries Group Member: Brian Burkhalter
Vote: Yes. -Chris On 27/07/2023 19:35, Roger Riggs wrote: |I hereby nominate |Brian Burkhalter|to Membership in the Core Libraries Group Brian has been working in the Core Library team since 2012. Brian's many contributions to the OpenJDK libraries include java.io subsystems, NIO channels, files, and buffers. He applies his background in math and image processing to OpenJDK development. ||Previously, he led JavaFX Media team. Co-designed and implemented successor Java audio and video playback and recording API using GStreamer, AV Foundation, and libav. Votes are due by August 11th, 2023. Only current Members of the |||Core Libraries Group| [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote|
Integrated: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
On Fri, 9 Jun 2023 19:44:32 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [cee5724d](https://github.com/openjdk/jdk/commit/cee5724d09b9ef9bd528fb721b756cb052265e3d) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 9 Jun 2023 and > was reviewed by Roger Riggs, Uwe Schindler and Paul Sandoz. > > Thanks! This pull request has now been integrated. Changeset: 73a9f486 Author:Chris Hegarty URL: https://git.openjdk.org/jdk21/commit/73a9f486ae36510de1ff462a45d6e88301bd92d5 Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property Reviewed-by: uschindler, rriggs Backport-of: cee5724d09b9ef9bd528fb721b756cb052265e3d - PR: https://git.openjdk.org/jdk21/pull/3
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
On Fri, 9 Jun 2023 19:44:32 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [cee5724d](https://github.com/openjdk/jdk/commit/cee5724d09b9ef9bd528fb721b756cb052265e3d) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 9 Jun 2023 and > was reviewed by Roger Riggs, Uwe Schindler and Paul Sandoz. > > Thanks! Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk21/pull/3#issuecomment-1587529121
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
On Fri, 9 Jun 2023 19:44:32 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [cee5724d](https://github.com/openjdk/jdk/commit/cee5724d09b9ef9bd528fb721b756cb052265e3d) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 9 Jun 2023 and > was reviewed by Roger Riggs, Uwe Schindler and Paul Sandoz. > > Thanks! @RogerRiggs @PaulSandoz this is a clean backport. Could you please add your review. Thanks. - PR Comment: https://git.openjdk.org/jdk21/pull/3#issuecomment-1586171692
RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
Hi all, This pull request contains a backport of commit [cee5724d](https://github.com/openjdk/jdk/commit/cee5724d09b9ef9bd528fb721b756cb052265e3d) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Chris Hegarty on 9 Jun 2023 and was reviewed by Roger Riggs, Uwe Schindler and Paul Sandoz. Thanks! - Commit messages: - Backport cee5724d09b9ef9bd528fb721b756cb052265e3d Changes: https://git.openjdk.org/jdk21/pull/3/files Webrev: https://webrevs.openjdk.org/?repo=jdk21=3=00 Issue: https://bugs.openjdk.org/browse/JDK-8309727 Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk21/pull/3.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/3/head:pull/3 PR: https://git.openjdk.org/jdk21/pull/3
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property [v3]
> A trivial use of the Vector API when run with the security manager and a
> domain that does not grant permissions fails with
> java.security.AccessControlException: access denied
> ("java.util.PropertyPermission"
> "jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK" "read").
>
> The fix it minimal, as consistent with other system property access in the
> JDK - just access the property while asserting privileged. Note: no explicit
> permission grant to the vector module is required, as it is in the boot
> loader.
>
> This is the only such security manager related issue I see in this code, and
> I have looked.
Chris Hegarty has updated the pull request incrementally with one additional
commit since the last revision:
use loopBound
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14392/files
- new: https://git.openjdk.org/jdk/pull/14392/files/4312aec1..5a4be97f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14392=02
- incr: https://webrevs.openjdk.org/?repo=jdk=14392=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14392.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14392/head:pull/14392
PR: https://git.openjdk.org/jdk/pull/14392
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property [v2]
On Fri, 9 Jun 2023 18:12:36 GMT, Paul Sandoz wrote: >> Chris Hegarty has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add at bug and remove newline > > test/jdk/jdk/incubator/vector/VectorRuns.java line 74: > >> 72: return a.length; >> 73: >> 74: int length = a.length & ~(species.length() - 1); > > Recommend: > Suggestion: > > int length = species.loopBound(a.length); Done, [5a4be97](https://github.com/openjdk/jdk/pull/14392/commits/5a4be97faa822a612dbb9d86cf9b6cf4d88483da) - PR Review Comment: https://git.openjdk.org/jdk/pull/14392#discussion_r1224679125
Integrated: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
On Fri, 9 Jun 2023 13:02:18 GMT, Chris Hegarty wrote:
> A trivial use of the Vector API when run with the security manager and a
> domain that does not grant permissions fails with
> java.security.AccessControlException: access denied
> ("java.util.PropertyPermission"
> "jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK" "read").
>
> The fix it minimal, as consistent with other system property access in the
> JDK - just access the property while asserting privileged. Note: no explicit
> permission grant to the vector module is required, as it is in the boot
> loader.
>
> This is the only such security manager related issue I see in this code, and
> I have looked.
This pull request has now been integrated.
Changeset: cee5724d
Author:Chris Hegarty
URL:
https://git.openjdk.org/jdk/commit/cee5724d09b9ef9bd528fb721b756cb052265e3d
Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod
8309727: Assert privileges while reading the
jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
Reviewed-by: rriggs, uschindler, psandoz
-
PR: https://git.openjdk.org/jdk/pull/14392
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property [v2]
> A trivial use of the Vector API when run with the security manager and a
> domain that does not grant permissions fails with
> java.security.AccessControlException: access denied
> ("java.util.PropertyPermission"
> "jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK" "read").
>
> The fix it minimal, as consistent with other system property access in the
> JDK - just access the property while asserting privileged. Note: no explicit
> permission grant to the vector module is required, as it is in the boot
> loader.
>
> This is the only such security manager related issue I see in this code, and
> I have looked.
Chris Hegarty has updated the pull request incrementally with one additional
commit since the last revision:
add at bug and remove newline
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14392/files
- new: https://git.openjdk.org/jdk/pull/14392/files/1e15af5a..4312aec1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14392=01
- incr: https://webrevs.openjdk.org/?repo=jdk=14392=00-01
Stats: 2 lines in 2 files changed: 1 ins; 1 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/14392.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14392/head:pull/14392
PR: https://git.openjdk.org/jdk/pull/14392
RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
A trivial use of the Vector API when run with the security manager and a domain
that does not grant permissions fails with
java.security.AccessControlException: access denied
("java.util.PropertyPermission" "jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK"
"read").
The fix it minimal, as consistent with other system property access in the JDK
- just access the property while asserting privileged. Note: no explicit
permission grant to the vector module is required, as it is in the boot loader.
This is the only such security manager related issue I see in this code, and I
have looked.
-
Commit messages:
- initial commit
Changes: https://git.openjdk.org/jdk/pull/14392/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14392=00
Issue: https://bugs.openjdk.org/browse/JDK-8309727
Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/14392.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14392/head:pull/14392
PR: https://git.openjdk.org/jdk/pull/14392
Re: RFR: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
On Fri, 9 Jun 2023 13:02:18 GMT, Chris Hegarty wrote:
> A trivial use of the Vector API when run with the security manager and a
> domain that does not grant permissions fails with
> java.security.AccessControlException: access denied
> ("java.util.PropertyPermission"
> "jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK" "read").
>
> The fix it minimal, as consistent with other system property access in the
> JDK - just access the property while asserting privileged. Note: no explicit
> permission grant to the vector module is required, as it is in the boot
> loader.
>
> This is the only such security manager related issue I see in this code, and
> I have looked.
@PaulSandoz We just ran into this yesterday,
https://github.com/elastic/elasticsearch/pull/96715. The change here is trivial.
test/jdk/jdk/incubator/vector/VectorRuns.java line 32:
> 30: * @modules jdk.incubator.vector
> 31: * @run main VectorRuns
> 32: * @run main/othervm/java.security.policy=empty_security.policy VectorRuns
I just added a minimal test here, so as not to otherwise disturb other areas.
This is sufficient to very the fix, and ensure that it does not reoccur.
test/jdk/jdk/incubator/vector/VectorRuns.java line 73:
> 71: return a.length;
> 72:
> 73: int length = a.length & ~(species.length() - 1);
pre existing test issue.
-
PR Comment: https://git.openjdk.org/jdk/pull/14392#issuecomment-1584549575
PR Review Comment: https://git.openjdk.org/jdk/pull/14392#discussion_r1224274145
PR Review Comment: https://git.openjdk.org/jdk/pull/14392#discussion_r1224275043
Re: The non-deterministic iteration order of Immutable Collections
Hi Stuart, First, thanks for you detailed reply. On 25/03/2023 00:16, Stuart Marks wrote: ... Yes, this has come up before, but it's been mostly theoretical. That is, people worry about this when they hear of the idea of randomized iteration order, but I've never heard any followup. This is in fact the first time I've heard of an actual case where this is a real problem. So thanks for bringing it up. Here is a link to the GH issue has some more details, and links to further issues and PR's. https://github.com/elastic/elasticsearch/issues/94946 (And unfortunately, it's notoriously difficult to make code truly iteration-order dependent. We've had our own history of problems with this in the JDK. I'd be interested in hearing from you at some point about the exact pathology of how this occurred.) There's currently no debugging or experimental interface that will let one print or set the random seed that's in use. The obvious approach of using an agent to hack away at runtime doesn't work, because the unmodifiable collections implementations are used very early in startup and they're usually loaded before the agent runs. There are limitations on what an agent can do when redefining an already-loaded class, for example, removing the 'final' modifier from fields isn't supported. (Well I suppose one can always use Unsafe.) Yes, unfortunately that is one of the options that we're considering :-( Here's another approach that might work for you. 1. Write a little program that extracts the class bytes of ImmutableCollection.class from the runtime image, and use something like ASM or ClassFile to make the class public and to make the REVERSE and SALT32L fields public and non-final. 2. Launch a VM using the --patch-module option to use this class instead of the built-in one. 3. Write an agent, or some debugging library that's called at the right time, to use simple reflective access to get or set those fields as desired. This is a bit fiddly but it might be easier than rebuilding and deploying a custom JDK. Yes, certainly a bit fiddly, and maybe a little fragile - we currently support compiling on JDK 17, with a strong preference of running on the latest JDK ( currently JDK 20 ). It could be that we'd need to maintain a version of this for 17 through 20 ( and 21, as we test with EA builds of 21 ). Setting (formerly) final fields after the VM is initialized is often somewhat risky. In this case these particular fields are used only during iteration, and they don't actually change the layout of any data structures. So setting them to some desired value should apply to all subsequent iterations, even of existing data structures. I'll think about better ways to do this in the product. The best approach isn't obvious. The typical way of doing things like this using system properties is tricky, as it depends on order of class initialization at startup (and you know how fragile that is). Yeah, the set of possible solutions is somewhat curtailed, but "the simpler the better"! ;-) Something like a system property would be "good enough", if the initialization order could be enforced. Thanks, -Chris.
The non-deterministic iteration order of Immutable Collections
I know that this has come up a number of times before, but I cannot seem to find anything directly relevant to my use-case, or recent. The iteration order of immutable Sets and Maps (created by the `of` factories) is clearly unspecified - good. Code should not depend upon this iteration order, and if so that code is broken. I completely agree and am aligned with this principle. The Elasticsearch code base heavily uses these collection implementations, and their iterators. On occasion we run into issues, where our code (because of a bug or a race), inadvertently has a dependency on the iteration order. To be clear, this is a bug in our code, and we want to reproduce and fix it. The problem we are encountering is that the iteration order is not determinable or reproducible, which makes it extremely challenging to reproduce the bug even when we have reproducible testcase to hand. ( whereas, for example, it is common practice in other parts of the code to log a seed used for randomization ) We very much like using the immutable collections, and we definitely don't want to pay the price of sorting things in the code, since we don't want to, and should not, depend upon the iteration order. The only concern is with reproducibility when we run into (our own) bugs. I don't (yet) want to be prescriptive in any potential solution. And I know that this has been discussed before. I mostly just want to start a conversation and see how much traction it gets. Thanks, -Chris.
JDK 20 EA builds (archive?)
Hi, While definitely not the right list, someone here will surely know ... Where can I download archive EA builds of JDK 20, so I can perform bisect experiments to help narrow when something changed between JDK 19 and JDK 20. ( I have some builds, but not that many ) --- Informational: I'm not expecting anyone to do my work for me ;-) but if you have ideas or hints! I'm try to track down a change in G1 GC behaviour, where the number of GC's of the Young generation has decreased ( by ~20% ), but the cumulative total time spent in GC of the Young generation has increased marginally ( ~2% ) for a particular workload benchmark ( that runs for a medium amount of time, ~20 mins ). Thanks, -Chris.
Re: RFR: 8303198: System and Runtime.exit() resilience to logging errors [v2]
On Tue, 28 Feb 2023 14:09:50 GMT, Alan Bateman wrote: >> But does that logging include the thread identity? If multiple threads can >> race to exit and all log, then the developer/user needs to know which >> logging came from which thread. > >> But does that logging include the thread identity? If multiple threads can >> race to exit and all log, then the developer/user needs to know which >> logging came from which thread. > > That's really up to the Logger and its configuration. If j.u.logging is used > then formatters can be configured to put the thread ID into the log records. > With 3rd party logging libraries there seems to be several choices, like %t > for the thread name. > > The main usage for this logging is to be able to find code in tests, plugins, > etc. that is calling System.exit and causing the test runner or container to > exit. So I think it's less about "which thread" and more about "which code". System.Logger is a facade for arbitrary user-code, commonly the log4J bridge. I routinely observe log4J throwing SecurityException's from deep within its implementation (since it is only partially implemented to handle the security manager) - PR: https://git.openjdk.org/jdk/pull/12770
Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v6]
On Fri, 17 Feb 2023 17:27:50 GMT, Roger Riggs wrote:
>> It can be difficult to find the cause of calls to
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> Roger Riggs 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 eight additional
> commits since the last revision:
>
> - Move test to the java/lang/RuntimeTests directory.
>Added implNote to System.exit.
>A few javadoc updates for review comments.
> - Merge branch 'master' into 8301627-log-system-exit
> - Improve implNote for Runtime.exit() with review suggestions.
> - Correct System.getLogger link
> - Add an @implNote to Runtime.exit to describe the java.lang.Runtime logging.
> - Added try/catch around lookup of logger so exceptions do not prevent
> System.exit.
>Added test case with console logger (when java.util.logging) not present.
>Removed @implNote tag its not appropriate in implementation javadoc.
>Still looking into when and where the log configuration should be
> described.
> - Locate the System logger before taking the shutdown lock
> - Add logging of calls to Runtime.exit to the system logger
> "java.lang.Runtime".
I left one trivial comment relating to a typo, but otherwise LGTM.
src/java.base/share/classes/java/lang/Runtime.java line 160:
> 158: *
> 159: * @implNote
> 160: * If the {@linkplain System#getLogger(String) the system logger}
> for {@code java.lang.Runtime}
Trivially, remove the double "the the" here.
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12517
Re: RFR: 8300228: ModuleReader.find on exploded module throws if resource name maps to invalid file path
On Tue, 17 Jan 2023 11:34:52 GMT, Alan Bateman wrote: > The ModuleReader implementation for exploded modules maps resource names to > file paths. A small oversight is that it doesn't handle InvalidPathException > which is thrown when the resource name maps to something that can't be parsed > as a file path. This has a knock on impact to Class/ClassLoader > getResourceXXX and other usages of ModuleReader. > > The existing ModuleReaderTest is updated to include additional resource names > to test. I've also moved `@Test` annotation to the test methods so that it's > a bit clearer which are the test methods in this source file. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/12035
Re: RFR: 8298567: Make field in RandomAccessFile final [v2]
On Mon, 12 Dec 2022 14:26:40 GMT, Per Minborg wrote: >> This PR proposes making a field in `RandomAccessFile` final. Also, it >> modernises a switch statement. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Make the field rw final LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11632
Re: RFR: 8294278: ForkJoinPool.getAndAddPoolIds should use Unsafe.staticFieldBase
On Fri, 25 Nov 2022 14:53:57 GMT, Alan Bateman wrote: > Two (since 19) usages of Unsafe.getAndAddInt to update a static field provide > the Class object as the base instead of the base object returned by > Unsafe.staticFieldBase. This doesn't change anything on the HotSpot VM. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11369
Re: RFR: 8297295: Remove ThreadGroup.allowThreadSuspension
On Fri, 25 Nov 2022 18:54:28 GMT, Alan Bateman wrote: > Another small step in the multi-release/multi-year effort to remove cruft > from Thread/ThreadGroup. > > java.lang.ThreadGroup.allowThreadSuspension(boolean) dates from JDK 1.1 and > the Classic VM. The method controlled whether threads were suspended when the > GC failed. It appears to have interacted with a callback mechanism that could > potentially free memory, allowing the GC to retry. The method was never > specified . > > The method was deprecated and changed to do nothing in JDK 1.2. It was > deprecated for removal in Java 14. > > A corpus analysis of 30M classes in 130k artifacts found 0 usages of this > method. > > It is time to finally remove this method. The compatibility impact should be > negligible. Joe, Stuart and I discussed briefly and think early in JDK 21 is > a good time to do this. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11373
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Wed, 23 Nov 2022 16:02:37 GMT, Chris Hegarty wrote: >> I would prefer to to avoid creating new factories when the desired function >> can be done on the resulting thread. >> Such as `setDaemon()` and `setName()`, etc. >> It does avoid the doPriv in this case, but is not necessary and when the >> security manager goes away, will leave around clutter (duplicated) >> functionality. > > Looking beyond this specific change, there is a lot of potential use for this > new factory elsewhere in the code. It also avoids similar bugs from possibly > reoccurring (by having the setDaemon call inside the factory). In the interest of making progress, let’s revert the change to the factory. This can be done separately, if at all. - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Sat, 26 Nov 2022 15:50:54 GMT, Ryan Ernst wrote: >> This commit guards thread modifications for the process reaper thread with >> doPrivileged. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > Revert factory method LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 05:01:40 GMT, Ryan Ernst wrote:
> This commit guards thread modifications for the process reaper thread with
> doPrivileged.
Changes requested by chegar (Reviewer).
src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java line 137:
> 135: public static Thread newSystemThread(String name, Runnable target,
> 136: long stackSize, int priority,
> 137: boolean daemon) {
Thanks for adding this overload, I think that it will be useful for the future
too. ( it never seems to matter how many variants of these factories we have,
we still need one more :-) )
-
PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 05:01:40 GMT, Ryan Ernst wrote: > This commit guards thread modifications for the process reaper thread with > doPrivileged. Hi @rjernst Thanks for taking this one on. I agree with adding the privileged blocks around the calls to Thread::setName, but the usage raises a warning which fails the build. It might be cleaner and easier to refactor into a privilegedThreadSetName helper method. Additionally, I noticed that there is an existing test that can be modified slightly to cover this. I've put these two comments / suggestions in the form of a PR and raised it against your branch. https://github.com/rjernst/jdk/pull/1 - PR: https://git.openjdk.org/jdk/pull/11309
Re: JDK 19 innocuous reaper threads
Thanks you Alan and Roger. I filed the following issue to track this: https://bugs.openjdk.org/browse/JDK-8297451 There are likely many more usages of innocuous thread that could be improved by ensuring setDaemon is invoked while asserting privileges, but I'd like to leave those to a later issue, since the usage in process is causing us issues right now, and it would be great to get this fixed (and eventually backported to 19.0.2). -Chris. On 22/11/2022 17:01, Roger Riggs wrote: Hi Chris, Yes, adding a doPriv for setDaemon and setName in a couple of places makes sense. Thanks, Roger On 11/22/22 11:12 AM, Chris Hegarty wrote: Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: JDK 19 innocuous reaper threads
Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
JDK 19 innocuous reaper threads
Hi, A change in JDK 19, that changed process reaper threads to be innocuous [1], has had an adverse affect when terminating the Elasticsearch server [2]. I agree with changing the process reapers to be innocuous, but just wonder if we're missing a few doPriv blocks. Additionally, and also in JDK 19, is the welcome improvement of the pid in the reaper thread name [3], but again I wonder if we're missing a few doPriv blocks here too. The issues we're seeing arise from the calls to `setDaemon` and `setName` outside of doPriv blocks, which can (depending on your security manager implementation) result in checking the callers permissions, and its callers permissions, etc, all the way to the thread's inherited access control context - which is effectively empty for these threads, since they are innocuous. I don't remember where we ended up with these kinda calls for innocuous threads, I'm thinking specifically about `setDaemon` (but `setName` seems similar) - whether they should be in doPriv or not, given the default security manager implementation. My feeling is that they should, since the caller should never be required to hold any specific permissions for these specific operations [4][5][6] to succeed. -Chris. [1] https://bugs.openjdk.org/browse/JDK-8279488 [2] https://github.com/elastic/elasticsearch/issues/91650 [3] https://bugs.openjdk.org/browse/JDK-8284165 [4] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#L103 [5] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL144 [6] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL175
Re: RFR: 8293810: Remove granting of RuntimePermission("stopThread") from tests
On Wed, 5 Oct 2022 15:01:15 GMT, Alan Bateman wrote:
> This is a test only change to remove the granting of
> RuntimePermission("stopThread") from the tests. With Thread.stop changed to
> throw UOE it means there is nothing that requires this permission.
LGTM
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10577
Re: RFR: 8285405: add test and check for negative argument to HashMap::newHashMap et al
On Tue, 9 Aug 2022 09:36:28 GMT, Jaikiran Pai wrote: > (This is a recreation of a previous pull request which had received some > reviews https://github.com/openjdk/jdk/pull/9036. I had to delete that > personal branch and recreate it due to some git issues) > > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285405? LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9806
Re: RFR: 8290504: Close streams returned by ModuleReader::list [v4]
On Thu, 21 Jul 2022 13:23:43 GMT, Ryan Ernst wrote: >> This commit ensures streams returned by ModuleReader::list are closed. > > Ryan Ernst has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains six commits: > > - Merge branch 'master' into try_files/module_reader_uses > - fix caller sensitive test uses > - silly spaces > - Merge branch 'master' into try_files/module_reader_uses > - revert CallerSensitiveAccess change > - 8290504: Close streams returned by ModuleReader::list > >This commit ensures streams returned by ModuleReader::list are closed. LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9557
Re: RFR: 8290504: Close streams returned by ModuleReader::list [v3]
On Wed, 20 Jul 2022 20:54:27 GMT, Ryan Ernst wrote: >> yes, this would solve it. > > Done in > [4a94c64](https://github.com/openjdk/jdk/pull/9557/commits/4a94c645fe1e37abc694f81fd8e401c5dc367d68) @rjernst it seems that the _revert_ part of the above has been done, but not the "have the callerSensitiveMethods DataProvider close the returned stream". - PR: https://git.openjdk.org/jdk/pull/9557
Re: RFR: 8290504: Close streams returned by ModuleReader::list
On Tue, 19 Jul 2022 14:07:17 GMT, Ryan Ernst wrote:
> This commit ensures streams returned by ModuleReader::list are closed.
test/jdk/java/lang/invoke/callerSensitive/CallerSensitiveAccess.java line 411:
> 409: try (ModuleReader reader = mref.open();
> 410: Stream stream = reader.list()) {
> 411: return stream
This change is causing the test to fail, in the `callerSensitiveMethods`
DataProvider, because the data provider is expecting an open stream to be
returned by `callerSensitiveMethods(Module)` - the stream is now closed.
There are a couple of ways to resolve this, but the most straightforward would
be to revert this part of the change, and have the `callerSensitiveMethods`
DataProvider close the returned stream. E.g.:
@DataProvider(name = "callerSensitiveMethods")
static Object[][] callerSensitiveMethods() {
try (var methodStream = callerSensitiveMethods(Object.class.getModule()))
{
return methodStream
.map(m -> new Object[]{m, shortDescription(m)})
.toArray(Object[][]::new);
}
}
-
PR: https://git.openjdk.org/jdk/pull/9557
Re: RFR: 8290353: ModuleReader::list specification should suggest closing the returned stream [v3]
On Tue, 19 Jul 2022 14:06:52 GMT, Ryan Ernst wrote: >> This commit adds additional clarification to the javadocs of >> ModuleReader::list about needing to close the stream. The new wording is >> similar to that used in Files::find and other similar methods. > > Ryan Ernst 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 five additional commits since > the last revision: > > - Merge branch 'master' into try_files/module_reader > - Revert "fix uses of list() in java.base" > >This reverts commit a72a5b9ec4e22ca4a414554b722042d4cbfaef58. > - adjust wording > - fix uses of list() in java.base > - 8290353: Clarify the streams returned by ModuleReader::list > >This commit adds additional clarification to the javadocs of >ModuleReader::list about needing to close the stream. The new wording is >similar to that used in Files::find and other similar methods. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9538
Re: RFR: 8290353: Clarify the streams returned by ModuleReader::list [v2]
On Mon, 18 Jul 2022 18:19:48 GMT, Ryan Ernst wrote: >> This commit adds additional clarification to the javadocs of >> ModuleReader::list about needing to close the stream. The new wording is >> similar to that used in Files::find and other similar methods. > > Ryan Ernst has updated the pull request incrementally with two additional > commits since the last revision: > > - adjust wording > - fix uses of list() in java.base The following CSR has been filed for this issue, https://bugs.openjdk.org/browse/JDK-8290521. Please review. - PR: https://git.openjdk.org/jdk/pull/9538
Re: RFR: 8290353: Clarify the streams returned by ModuleReader::list [v2]
On Mon, 18 Jul 2022 19:48:02 GMT, Mark Reinhold wrote: > It’s odd to mix adding some advice to the Javadoc with adopting that advice > in the code base. Consider opening a new issue and PR for the actual code > changes. [JDK-8290504][1] has been filed to track the implementation changes. @rjernst please separate out the implementation changes into a separate PR, using 8290504 as the linked JIRA issue. (apologies, I previously asked for the implementation changes to be in this PR ) > Also, a better summary for this issue (and PR) would be “ModuleReader::list > specification should suggest closing the returned stream,” since that’s the > essence of it. Agreed. I updated the JIRA issue summary as suggested. @rjernst please update the title of this PR to match. [1]: https://bugs.openjdk.org/browse/JDK-8290504 - PR: https://git.openjdk.org/jdk/pull/9538
Re: RFR: 8290359: Ensure that all directory streams are closed in jdk.link [v2]
On Mon, 18 Jul 2022 18:22:02 GMT, Ryan Ernst wrote: >> This commit adds try-with-resources for uses of Stream from Files >> methods that walk directories. > > Ryan Ernst 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 try_files/jdk.link > - fix alignment > - 8290359: Ensure that all directory streams are closed in jdk.link > >This commit adds try-with-resources for uses of Stream from Files >methods that walk directories. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9520
Re: RFR: 8290359: Ensure that all directory streams are closed in jdk.link
On Fri, 15 Jul 2022 16:18:17 GMT, Ryan Ernst wrote:
> This commit adds try-with-resources for uses of Stream from Files
> methods that walk directories.
Changes requested by chegar (Reviewer).
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 834:
> 832: name.endsWith(".EC") ||
> 833: name.startsWith("META-INF/SIG-")
> 834: );
Trivially, can we please keep the indentation consistent with the previous
version. So, align all `name.endsWith` expressions under the 's' from
startsWith.
-
PR: https://git.openjdk.org/jdk/pull/9520
Re: RFR: 8290316: Ensure that all directory streams are closed in java.base [v3]
On Mon, 18 Jul 2022 14:02:47 GMT, Ryan Ernst wrote: >> This commit guards uses of Files methods returning path streams in >> java.base to use try-with-resources. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > fix compile and address more feedback LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9518
Re: RFR: 8290353: Clarify the streams returned by ModuleReader::list
On Mon, 18 Jul 2022 14:06:00 GMT, Ryan Ernst wrote: > This commit adds additional clarification to the javadocs of > ModuleReader::list about needing to close the stream. The new wording is > similar to that used in Files::find and other similar methods. Given the recommendation to close streams returned by the `list` method, then this PR could be extended to update any usages in, at least, java.base. If there are many usages beyond java.base, then a separate issue could be filed to update them too. ( if not already covered by a previous PR in this area ) - PR: https://git.openjdk.org/jdk/pull/9538
Re: RFR: 8290353: Clarify the streams returned by ModuleReader::list
On Mon, 18 Jul 2022 14:06:00 GMT, Ryan Ernst wrote: > This commit adds additional clarification to the javadocs of > ModuleReader::list about needing to close the stream. The new wording is > similar to that used in Files::find and other similar methods. src/java.base/share/classes/java/lang/module/ModuleReader.java line 217: > 215: * > 216: * The returned stream contains references to one or more open > directories > 217: * in the module. The directories are closed by closing the stream. > This mostly looks good - I agree with adding the note here. Should the wording be relaxed a little - replace "contains references" with "may contain references" - since the stream will not always contain references to open directories - it depends on the type of reader. The end result / guidance is the same - use in a try-with-resources block - I'm just suggesting a slight relaxing of the wording. src/java.base/share/classes/java/lang/module/ModuleReader.java line 224: > 222: * @apiNote > 223: * This method must be used within a try-with-resources statement or > similar > 224: * control structure to ensure that the stream's open directories > are closed Similar to my other comment - should "must' be replaced with "should" - for the same aforementioned reason. - PR: https://git.openjdk.org/jdk/pull/9538
Re: RFR: 8290316: Ensure that all directory streams are closed in java.base [v2]
On Sat, 16 Jul 2022 03:35:06 GMT, Bernd wrote:
>> Ryan Ernst has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> address formatting feedback
>
> src/java.base/share/classes/java/time/chrono/HijrahChronology.java line 1041:
>
>> 1039: try (Stream stream = Files.list(CONF_PATH)) {
>> 1040: stream.map(p -> p.getFileName().toString())
>> 1041: .filter(fn ->
>> fn.matches("hijrah-config-[^\\.]+\\.properties"))
>
> BTW Should this use RESOURCE_PREFIX/SUFFIX and is the \\ inside [character
> class] suppose to quote the .?
@ecki this seems orthogonal to this particular issue. Maybe it could be
considered separately, if needed.
-
PR: https://git.openjdk.org/jdk/pull/9518
Re: RFR: 8290316: Ensure that all directory streams are closed in java.base [v2]
On Fri, 15 Jul 2022 19:33:56 GMT, Ryan Ernst wrote:
>> This commit guards uses of Files methods returning path streams in
>> java.base to use try-with-resources.
>
> Ryan Ernst has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address formatting feedback
Changes requested by chegar (Reviewer).
src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 262:
> 260: p = module.relativize(p);
> 261: String pkgName = slashesToDots(p.toString());
> 262: // skip META-INFO and empty strings
Trivially, while here can we fix this typo, META-INFO <- drop the `O`
src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 271:
> 269: moduleNames.add(moduleName);
> 270: }
> 271: });
There is a missing closing curly brace here (which should come on the next
line), but otherwise looks fine.
src/java.base/share/classes/jdk/internal/module/ModulePatcher.java line 138:
> 136: stream.filter(path -> (!isAutomatic
> 137: || path.toString().endsWith(".class"))
> 138: && !isHidden(path))
Trivially, can the indentation of the two boolean operators be pushed in so
that they align underneath the lambda parameter.
-
PR: https://git.openjdk.org/jdk/pull/9518
Re: RFR: 8289768: Clean up unused code [v3]
On Fri, 8 Jul 2022 07:08:46 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - Update copyright > - Remove more unused variables LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/9383
Re: [jdk19] RFR: 8289872: wrong wording in @param doc for HashMap.newHashMap et. al.
On Thu, 7 Jul 2022 06:06:42 GMT, Stuart Marks wrote: > Minor doc wording changes, to be consistent with HashSet.newHashSet et. al. LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk19/pull/118
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v2]
On Mon, 4 Jul 2022 12:19:27 GMT, David Holmes wrote: >> Ryan Ernst has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better clarify multiple threads behavior > >> Let's say we've run shutdown so run all the hooks but not halted. Then >> someone calls exit(0). That seems to suggest the call will block >> indefinitely, which is neither desirable nor what was actually implemented. > > If the hook threads do not halt then the exiting thread (which holds the > lock) blocks forever in the join(). Any other call to exit blocks trying to > acquire the lock. That has always been the way this works - if hook threads > don't terminate then the VM doesn't (unless someone calls halt() directly). > That is one of the things the window you suggested be closed, allowed - a > call to exit(non-zero) could force a call to halt(). @dholmes-ora @AlanBateman @kimbarrett The following [CSR](https://bugs.openjdk.org/browse/JDK-8289824) has been filed to cover the specification related changes in this PR - please review as appropriate. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v5]
On Tue, 5 Jul 2022 18:43:26 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst 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 five additional commits since > the last revision: > > - Merge branch 'master' into shutdown_cleanup > - iter text > - iterate on wording > - better clarify multiple threads behavior > - 8288984: Simplification in Shutdown.exit > >This is a followup to simplify Shutdown.exit after the removal of >finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >on the approach has been reached in this PR, a CSR will be filed to >propose the spec change to Runtime.exit. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v4]
On Tue, 5 Jul 2022 13:30:40 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > iter text Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Tue, 5 Jul 2022 07:23:49 GMT, David Holmes wrote:
>> I like the new suggested wording, but I would slightly tweak it. As Alan
>> mentioned in another comment the first paragraph makes clear the method
>> never returns normally. How does the following sound?
>>
>>> If this method is invoked after shutdown hooks have already been started,
>>> the supplied status code will be ignored. If this method is invoked from a
>>> shutdown hook the system will deadlock.
>
>> gives the impression that an invocation of this method WILL terminate the VM
>> with the given status code - which is not actually true, given the potential
>> for signals.
>
> The subtle distinction being calling Runtime.exit versus Shutdown.exit. The
> actual serialization takes places in Shutdown.exit and serializes with
> signals too. But discussion of signals has no place here - I'm not even sure
> we document those ways of terminating the VM anywhere?
@dholmes-ora Agreed - this paragraph should not explicitly mention signals or
any _other_ means of termination.
There is some shutdown hook specific verbiage relating to termination in
`Runtime::addShutdownHook` :
In rare circumstances the virtual machine may abort, that is, stop running
without shutting
down cleanly. This occurs when the virtual machine is terminated externally,
for example
with the SIGKILL signal on Unix or the TerminateProcess call on Microsoft
Windows. The
virtual machine may also abort if a native method goes awry by, for example,
corrupting
internal data structures or attempting to access nonexistent memory. If the
virtual machine
aborts then no guarantee can be made about whether or not any shutdown hooks
will be run.
... but we do not need to go there, and it's not really enough to leverage
anyway.
>The subtle distinction being calling Runtime.exit versus Shutdown.exit. The
>actual serialization takes places in Shutdown.exit and serializes with signals
>too.
Exactly. On further reading of the proposed "Invocations of this method are
serialized such that only one invocation will actually proceed with the
shutdown sequence and terminate the VM with the given status code.", I think
that it is fine (given its position and context in Runtime::exit). My reason
for suggesting an alternative ("If this method is invoked after shutdown hooks
have already been started, it will block indefinitely") was mainly to try to
find something a little simpler while retaining correctness. Given the
discussion so far, either is fine with me.
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:31:22 GMT, Alan Bateman wrote: >> I reworded with the suggested text in mind. I also added another sentence >> referencing native signal handlers, which may also initiate shutdown. I >> think this clarification is important so that it is clear the status code of >> all invocations from Java may still be ignored. See >> [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). > > I think the wording in the latest commit (9d972b) is problematic. One reason > is that you've changed it to "will run shutdown hooks" but it doesn't run the > hooks, it starts them, and so conflicts with the previous paragraph. I also > think "That invocation may be initiated via platform specific signal > handlers" is confusing here as there is no notion of "signal handler" to > reference. There may be scope for text elsewhere, maybe with an implNote for > signals such as HUP, but I don't think this is the PR for this. So I think it > better to try the wording that David suggested and see if it needs any > improvement. YAO (Yet Another Opinion)! But I think we're actually converging. David's wording: >Invocations of this method are serialized such that only one invocation will >actually proceed with the shutdown sequence and terminate the VM with the >given status code. ... gives the impression that an invocation of this method WILL terminate the VM with the given status code - which is not actually true, given the potential for signals. This is already alluded to in `Runtime::addShutdownHook`. I agree that this could be resolved separately, but we should _allow_ for it. The original wording: > If this method is invoked after all shutdown hooks have already been run ... ... seems quite clever (and allows for signals). Maybe we could: > If this method is invoked after shutdown hooks have already been started it > will block indefinitely. If this method is invoked from a shutdown hook the > system will deadlock. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 01:57:11 GMT, Kim Barrett wrote:
> Is "deadlock" accurate?
Yes.
In the context of the specification, "shutdown hook" means _application_
shutdown hook - as far as the specification is concerned, application shutdown
hooks are the only kind of hooks. Right?
For example, the following will deadlock (when run with the changes in this PR):
public class TestHook {
public static void main(String... arg) {
Thread hook = new Thread("my-hook") {
@Override
public void run() {
System.exit(1);
}
};
Runtime.getRuntime().addShutdownHook(hook);
System.exit(0);
}
}
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit
On Sat, 2 Jul 2022 13:23:27 GMT, David Holmes wrote: > Sorry, I did not think this issue was intended to change the specification in > any way, but I see now that it actually does - whether that was the intent or > not. While, maybe, not strictly envisioned by the JIRA issue, the specification clarification is IMO a good addition. We should update the synopsis to make it clear that the simplification is to j.l.Runtime::exit. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line 1202:
> 1200: */
> 1201: public boolean exists(Path path, LinkOption... options) {
> 1202: try {
Overall, I think that this is a great change (avoiding the need for various
parts of the system to communicate through exceptions).
Trivially, and easily, I often find it useful to add mock-like tests for these
specified implementations (implSpec), i.e. just subclass FSP mocking out the
abstract methods to ensure that the default implementations invoke the
appropriate ones.
-
PR: https://git.openjdk.org/jdk/pull/9249
