Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v13]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Merge branch 'master' into remove-finalizers - reference emunCtx and homeCtx consistently in constructor - Restore EnumCtx to being an inner class, to keep all the state together in the code - Restore comments in ldap capture file - Update test files - add copyright, etc - added getters to EnumCtx, and moved it to top level ; use getters + local variables to reduce code changes - test comment udpate - Update .ldap capture file - Check that cleanup was performed correctly - Merge branch 'master' into remove-finalizers - ... and 13 more: https://git.openjdk.java.net/jdk/compare/b7a34f72...afd1c9b5 - Changes: https://git.openjdk.java.net/jdk/pull/8311/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8311=12 Stats: 623 lines in 6 files changed: 352 ins; 103 del; 168 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v12]
On Tue, 7 Jun 2022 18:45:17 GMT, Roger Riggs wrote: > The commented out printf/println's should be removed before committing. Do you mean the pre-existing `println`s in LdapSearchEnumeration.java? - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v12]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Restore EnumCtx to being an inner class, to keep all the state together in the code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/0f729619..c8bdac44 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=10-11 Stats: 90 lines in 1 file changed: 44 ins; 45 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v11]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with two additional commits since the last revision: - Restore comments in ldap capture file - Update test files - add copyright, etc - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/e34e888f..0f729619 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=09-10 Stats: 44 lines in 1 file changed: 33 ins; 1 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
On Fri, 3 Jun 2022 18:05:52 GMT, Mandy Chung wrote: > This reapplies JDK-8287384 and adjust the number of GCs for negative case, > i.e. the condition is never met that is used to verify a reference is not > GC'ed. Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9021
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
On Wed, 1 Jun 2022 21:25:25 GMT, Roger Riggs wrote:
>> Brent Christian has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 14 commits:
>>
>> - Merge branch 'master' into remove-finalizers
>> - Threading-related fixes
>> - NOW it builds
>> - Fix the merge fix
>> - Fix merge
>> - Merge
>> - Rename inner class to EnumCtx ; use homeCtx() in
>> AbstractLdapNamingEnumeration for consistencty ; new instance vars can be
>> final
>> - fix whitespace
>> - Merge branch 'master' into remove-finalizers
>> - Test changes to test new cleaner code
>> - ... and 4 more:
>> https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff
>
> src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
> line 176:
>
>> 174: LdapResult newRes =
>> homeCtx().getSearchReply(enumCtx.enumClnt, enumCtx.res);
>> 175: enumCtx.setRes(newRes);
>> 176: if (enumCtx.res == null) {
>
> This looks odd, setting the value using synchronized, but reading it without.
I've added getters to EnumCtx, and a comment explaining why only setters are
synchronized.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v10]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with two additional commits since the last revision: - added getters to EnumCtx, and moved it to top level ; use getters + local variables to reduce code changes - test comment udpate - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/3941e6f1..e34e888f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=08-09 Stats: 125 lines in 5 files changed: 50 ins; 36 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
On Fri, 27 May 2022 22:09:22 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 14 commits: > > - Merge branch 'master' into remove-finalizers > - Threading-related fixes > - NOW it builds > - Fix the merge fix > - Fix merge > - Merge > - Rename inner class to EnumCtx ; use homeCtx() in > AbstractLdapNamingEnumeration for consistencty ; new instance vars can be > final > - fix whitespace > - Merge branch 'master' into remove-finalizers > - Test changes to test new cleaner code > - ... and 4 more: > https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff I've updated the test case to confirm that cleanup is performed correctly, by inspecting the enumCount within the LdapCtx. - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v9]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with two additional commits since the last revision: - Update .ldap capture file - Check that cleanup was performed correctly - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/4af66bff..3941e6f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=07-08 Stats: 86 lines in 2 files changed: 34 ins; 38 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge branch 'master' into remove-finalizers - Threading-related fixes - NOW it builds - Fix the merge fix - Fix merge - Merge - Rename inner class to EnumCtx ; use homeCtx() in AbstractLdapNamingEnumeration for consistencty ; new instance vars can be final - fix whitespace - Merge branch 'master' into remove-finalizers - Test changes to test new cleaner code - ... and 4 more: https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff - Changes: https://git.openjdk.java.net/jdk/pull/8311/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8311=07 Stats: 579 lines in 6 files changed: 309 ins; 101 del; 169 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v7]
On Fri, 27 May 2022 22:00:24 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Threading-related fixes `AbstractLdapEnumeration`'s mutable state brings the possibility of threading issues between the program and cleaner threads. I've added some threading-related changes to the fix. See my [comment in the bug report](https://bugs.openjdk.java.net/browse/JDK-8283660?focusedCommentId=14498566=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14498566) for additional background. Since synchronization may now happen on the cleaner thread, I've changed `AbstractLdapEnumeration` to use its own `Cleaner` instance instead of the shared cleaner, for added safety. There have been deadlocks in ldap cleanup in the past. The added `finally` blocks led to a lot of indentation changes. The "hide whitespace" option might help with viewing the changes. - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v7]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Threading-related fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/a48a56cc..ef8c1410 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=05-06 Stats: 371 lines in 5 files changed: 122 ins; 87 del; 162 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v6]
On Sat, 23 Apr 2022 00:09:48 GMT, Brent Christian wrote:
>> src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
>> line 73:
>>
>>> 71: public void run() {
>>> 72: if (enumClnt != null) {
>>> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
>>
>> It's a bit strange to see that there is no guard here to verify that
>> `homeCtx != null`, when line 76 implies that it might. My reading is that
>> `homeCtxt` is not supposed to be `null` when `enumClnt` is not `null`. That
>> could be explained in a comment, or alternatively asserted just before line
>> 73 (`assert homeCtx != null;`)
>
> Yes, it is strange -- that code came from the finalizer. I will add an assert.
It appears that the update() method can set 'homeCtx' for 'ne' to null while
leaving 'enumClnt' non-null (~L410).
Perhaps here, the clearSearchReply() call should only happen if both are
non-null.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v6]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: NOW it builds - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/9ffaa788..a48a56cc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v5]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Fix the merge fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/6398e5e5..9ffaa788 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v4]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Fix merge - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/9d19775b..6398e5e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v3]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge - Rename inner class to EnumCtx ; use homeCtx() in AbstractLdapNamingEnumeration for consistencty ; new instance vars can be final - fix whitespace - Merge branch 'master' into remove-finalizers - Test changes to test new cleaner code - Rename test to LdapEnumeration - Create copy of AddNewEntry test to test AbstractLdapNamingEnumeration Cleaner - Merge branch 'master' into remove-finalizers - Replace AbstractLdapNamingEnumeration finalizer with Cleaner - Changes: https://git.openjdk.java.net/jdk/pull/8311/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8311=02 Stats: 262 lines in 7 files changed: 194 ins; 20 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v2]
> Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Rename inner class to EnumCtx ; use homeCtx() in AbstractLdapNamingEnumeration for consistencty ; new instance vars can be final - Changes: - all: https://git.openjdk.java.net/jdk/pull/8311/files - new: https://git.openjdk.java.net/jdk/pull/8311/files/3c957b51..1df8515b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8311=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8311=00-01 Stats: 30 lines in 1 file changed: 1 ins; 0 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
On Wed, 20 Apr 2022 13:48:21 GMT, Daniel Fuchs wrote:
>> Please review this change to replace the finalizer in
>> `AbstractLdapNamingEnumeration` with a Cleaner.
>>
>> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
>> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
>> there, the change is fairly mechanical.
>>
>> Details of note:
>> 1. Some operations need to change the state values (the update() method is
>> probably the most interesting).
>> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
>> `homeCtx` from the superclass's `state`.
>>
>> The test case is based on a copy of
>> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test
>> case might be possible, but this was done for expediency.
>>
>> The test only confirms that the new Cleaner use does not keep the object
>> reachable. It only tests `LdapSearchEnumeration` (not
>> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are
>> subclasses of `AbstractLdapNamingEnumeration`).
>>
>> Thanks.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
> line 73:
>
>> 71: public void run() {
>> 72: if (enumClnt != null) {
>> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
>
> It's a bit strange to see that there is no guard here to verify that `homeCtx
> != null`, when line 76 implies that it might. My reading is that `homeCtxt`
> is not supposed to be `null` when `enumClnt` is not `null`. That could be
> explained in a comment, or alternatively asserted just before line 73
> (`assert homeCtx != null;`)
Yes, it is strange -- that code came from the finalizer. I will add an assert.
> src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
> line 83:
>
>> 81: }
>> 82:
>> 83: private CleaningAction state;
>
> I wonder if state should be final?
Makes sense to me. `cleanable` can be final, too.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
Please review this change to replace the finalizer in `AbstractLdapNamingEnumeration` with a Cleaner. The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult res`, and `LdapClient enumClnt`) are moved to a static inner class . From there, the change is fairly mechanical. Details of note: 1. Some operations need to change the state values (the update() method is probably the most interesting). 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read `homeCtx` from the superclass's `state`. The test case is based on a copy of `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case might be possible, but this was done for expediency. The test only confirms that the new Cleaner use does not keep the object reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are subclasses of `AbstractLdapNamingEnumeration`). Thanks. - Commit messages: - fix whitespace - Merge branch 'master' into remove-finalizers - Test changes to test new cleaner code - Rename test to LdapEnumeration - Create copy of AddNewEntry test to test AbstractLdapNamingEnumeration Cleaner - Merge branch 'master' into remove-finalizers - Replace AbstractLdapNamingEnumeration finalizer with Cleaner Changes: https://git.openjdk.java.net/jdk/pull/8311/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8311=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283660 Stats: 259 lines in 7 files changed: 192 ins; 20 del; 47 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Integrated: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
On Tue, 22 Mar 2022 17:59:15 GMT, Brent Christian wrote: > Please review this change to the java/util/prefs/AddNodeChangeListener.jar > test. > > Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh > Preferences on each test run, MacOS does not seem to honor this, and still > stores prefs in Library/. > > This test has long suffered intermittent failures. 8255031 added some > debugging code; as of that change, the test fails fast as soon as an expected > change event is not received. In particular, if the expected event is not > received for adding the node, the test exits without removing the node. In > this way, on Mac, the node can get "stuck" in the preferences of the machine. > Future runs on the machine will already have the node, no node added change > event will be generated (because the node was already there), the test will > continue to fail without removing the node, etc. This matches observations on > some CI machines. > > This change ensures that: > * the test node is not present before the test > * the test runs to completion, including removing the test node > * the test node is not left behind after the test > > Thanks. This pull request has now been integrated. Changeset: 656cba7a Author:Brent Christian URL: https://git.openjdk.java.net/jdk/commit/656cba7af376d6460202591230ac95d2366de9f3 Stats: 70 lines in 1 file changed: 30 ins; 17 del; 23 mod 8283349: Robustness improvements to java/util/prefs/AddNodeChangeListener.jar Reviewed-by: dfuchs, naoto, lancea - PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar [v2]
On Wed, 23 Mar 2022 17:29:04 GMT, Naoto Sato wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add test failure message to 'couldn't remove' RuntimeException
>
> test/jdk/java/util/prefs/AddNodeChangeListener.java line 63:
>
>> 61: } finally {
>> 62: // Make sure test node is not present after the test
>> 63: clearPrefs();
>
> If this call throws a `RuntimeException("Unable to clear...")`, it will
> override the possible `RuntimeException("Failed")` in the `try` block, but I
> think it is OK.
That's true - I hadn't considered that. I think it's OK, too. Even so, I've
added a message to the exception thrown from clearPrefs() to indicate if the
test failed.
-
PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar [v2]
> Please review this change to the java/util/prefs/AddNodeChangeListener.jar > test. > > Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh > Preferences on each test run, MacOS does not seem to honor this, and still > stores prefs in Library/. > > This test has long suffered intermittent failures. 8255031 added some > debugging code; as of that change, the test fails fast as soon as an expected > change event is not received. In particular, if the expected event is not > received for adding the node, the test exits without removing the node. In > this way, on Mac, the node can get "stuck" in the preferences of the machine. > Future runs on the machine will already have the node, no node added change > event will be generated (because the node was already there), the test will > continue to fail without removing the node, etc. This matches observations on > some CI machines. > > This change ensures that: > * the test node is not present before the test > * the test runs to completion, including removing the test node > * the test node is not left behind after the test > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Add test failure message to 'couldn't remove' RuntimeException - Changes: - all: https://git.openjdk.java.net/jdk/pull/7908/files - new: https://git.openjdk.java.net/jdk/pull/7908/files/71ab1f17..139b6a94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7908=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7908=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7908.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7908/head:pull/7908 PR: https://git.openjdk.java.net/jdk/pull/7908
RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
Please review this change to the java/util/prefs/AddNodeChangeListener.jar test. Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh Preferences on each test run, MacOS does not seem to honor this, and still stores prefs in Library/. This test has long suffered intermittent failures. 8255031 added some debugging code; as of that change, the test fails fast as soon as an expected change event is not received. In particular, if the expected event is not received for adding the node, the test exits without removing the node. In this way, on Mac, the node can get "stuck" in the preferences of the machine. Future runs on the machine will already have the node, no node added change event will be generated (because the node was already there), the test will continue to fail without removing the node, etc. This matches observations on some CI machines. This change ensures that: * the test node is not present before the test * the test runs to completion, including removing the test node * the test node is not left behind after the test Thanks. - Commit messages: - Ensure test node does not already exist, and is always removed Changes: https://git.openjdk.java.net/jdk/pull/7908/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7908=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283349 Stats: 70 lines in 1 file changed: 30 ins; 17 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/7908.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7908/head:pull/7908 PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v3]
On Fri, 25 Feb 2022 11:08:49 GMT, Claes Redestad wrote: >> Splitting out these micro changes from #7231 >> >> - Clean up and simplify setup and code >> - Add variants with different inputs with varying lengths and encoding >> weights, but also relevant mixes of each so that we both cover interesting >> corner cases but also verify that performance behave when there's a >> multitude of input shapes. Both simple and mixed variants are interesting >> diagnostic tools. >> - Drop all charsets from the default run configuration except UTF-8. >> Motivation: Defaults should give good coverage but try to keep runtimes at >> bay. Additionally if the charset tested can't encode the higher codepoints >> used in these micros the results might be misleading. If you for example >> test using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have >> all been replaced by `?`, so the test is effectively the same as testing >> ASCII-only. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix typos, add missing short variants to latin1 and utf16 short decode > methods > - Increase coverage of and weight of short strings Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks
On Thu, 17 Feb 2022 15:11:11 GMT, Claes Redestad wrote:
> Splitting out these micro changes from #7231
>
> - Clean up and simplify setup and code
> - Add variants with different inputs with varying lengths and encoding
> weights, but also relevant mixes of each so that we both cover interesting
> corner cases but also verify that performance behave when there's a multitude
> of input shapes. Both simple and mixed variants are interesting diagnostic
> tools.
> - Drop all charsets from the default run configuration except UTF-8.
> Motivation: Defaults should give good coverage but try to keep runtimes at
> bay. Additionally if the charset tested can't encode the higher codepoints
> used in these micros the results might be misleading. If you for example test
> using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have all
> been replaced by `?`, so the test is effectively the same as testing
> ASCII-only.
Changes requested by bchristi (Reviewer).
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 36:
> 34: @Warmup(iterations = 5, time = 2)
> 35: @Measurement(iterations = 5, time = 3)
> 36: @State(Scope.Thread)
No change to @Fork and @Warmup (as in the Encode test)?
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 49:
> 47: public class StringDecode {
> 48:
> 49: @Param({"US-ASCII", "ISO-8859-1", "UTF-8", "MS932", "ISO-8859-6",
> "ISO-2022-KR"})
What would you think of retaining the previous set of charset names as a
comment -- as a suggestion for someone who wants additional coverage?
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 93:
> 91: public void decodeAsciiLong(Blackhole bh) throws Exception {
> 92: bh.consume(new String(longAsciiString, charset));
> 93: bh.consume(new String(longAsciiString, 0, 1024 + 31, charset));
I imagine the 1024+31 addition gets compiled down, and is not executed during
the test, right?
test/micro/org/openjdk/bench/java/lang/StringEncode.java line 39:
> 37: public class StringEncode {
> 38:
> 39: @Param({"US-ASCII", "ISO-8859-1", "UTF-8", "MS932", "ISO-8859-6"})
Same, re: keeping list as a comment.
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v7]
On Wed, 16 Feb 2022 21:39:20 GMT, Tim Prinzing wrote: >> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is >> null > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge master > - fix test breakage from rename > - reformat test summary > - reformat test summary > - improve test name and remove experimental test code > - fix copyright date > - More changes from feedback. > >The javadoc is improved to reflect InvalidCallerException is thrown with >a caller that can't be assigned to a ClassLoader as well as a null >caller frame. Added a test IsParallelCapableBadCaller that uses >reflection hackery to create a case where called with an invalid caller >on the call stack. > - Changes from feedback. > >- Copyright dates fixed >- IllegalCallerException thrown for no caller frame, and associated >javadoc changes >- test changed to look for IllegalCallerException thrown. > - Merge branch 'master' into JDK-8281000 > - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Looks fine. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 22:37:57 GMT, Mandy Chung wrote: > Thanks for taking on these null caller issue. > > To give more context to this issue, the spec of > `ClassLoader::isRegisteredAsParallelCapable` returns true if this class > loader is registered as parallel capable, otherwise false. The current spec > does not specify what if the caller class is not a class loader. The current > implementation throws NPE if caller is null. I initially proposed to return > false for simplicity. However, if the caller is not a subclass of > `ClassLoader`, the current implementation throws `ClassCastException`. Both > cases are invalid caller and they should be handled in the same way, either > return false or throw an exception. > > Having a second thought, since this API expects to be called by a class > loader, I think throwing `IllegalCallerException` to indicate this method is > called by an illegal caller. This will need a CSR due to the spec change. > > What do you think? Throwing IllegalCallerException sounds good to me. As a static method, from a language standpoint, the call could appear almost anywhere. But its intended usage is pretty narrow. I think it's worth notifying the developer of a pretty obvious error. Is this method mainly meant to be called from a classloader's static initializer? That might be worth mentioning in the JavaDoc (if we're doing a CSR anyway...). - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v4]
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Brent Christian has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 34 commits:
- Merge branch 'master' into 8276447
- Merge branch 'master' into 8276447
- Merge branch 'master' into 8276447
- merge
- @SuppressWarnings(removal) in Windows CKey.java
- Add jls tag to runFinalization methods
- Update wording of Object.finalize, new paragraph is now bold
- update Object.finalize javadoc
- update Object.finalize JavaDoc and @deprecated tag
- Update Object.finalize deprecation message
- ... and 24 more: https://git.openjdk.java.net/jdk/compare/ea8d3c92...0a983d51
-
Changes: https://git.openjdk.java.net/jdk/pull/6465/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6465=03
Stats: 194 lines in 62 files changed: 54 ins; 38 del; 102 mod
Patch: https://git.openjdk.java.net/jdk/pull/6465.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6465/head:pull/6465
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai wrote:
> Can I please get a review for this change which proposes to fix the issue
> reported in https://bugs.openjdk.java.net/browse/JDK-8003417?
>
> The issue notes that this is applicable for `WeakHashMap` which have `null`
> keys. However, the issue is even applicable for `WeakHashMap` instances which
> don't have `null` keys, as reproduced and shown by the newly added jtreg test
> case in this PR.
>
> The root cause of the issue is that once the iterator is used to iterate till
> the end and the `remove()` is called, then the
> `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the
> key to remove from the map, instead of the key of the last returned entry.
> The commit in this PR fixes that part.
>
> A new jtreg test has been added which reproduces the issue as well as
> verifies the fix.
> `tier1` testing and this new test have passed after this change. However, I
> guess this will require a JCK run to be run too, perhaps? If so, I will need
> help from someone who has access to them to have this run against those
> please.
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/util/WeakHashMap.java line 826:
> 824: throw new ConcurrentModificationException();
> 825:
> 826: WeakHashMap.this.remove(lastReturned.get());
Unlike `currentKey`, `HashIterator` does not necessarily maintain a strong
reference to the referent of `lastReturned`. If the `lastReturned`
WeakReference were cleared between `lastReturned` being set, and the `remove()`
call, the null key could again be erroneously removed. It would be cool to add
a test case for this (maybe using large objects as keys would tempt the GC to
clear the weakrefs via `jdk.test.lib.util.ForceGC`).
Since we store the `NULL_KEY` sentinel value for the null key, I believe
`Entry.get()` will only return null in the case that the weakref has been
cleared. So could we instead fix this with:
if (lastReturned.get() != null) {
WeakHashMap.this.remove(lastReturned.get());
}
?
Or, if we're worried about the ref being cleared between the null check and
remove(), maybe:
Object lastReturnedKey = lastReturned.get();
if (lastReturnedKey != null) {
WeakHashMap.this.remove(lastReturned.get());
}
-
PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source [v2]
On Thu, 2 Dec 2021 02:36:47 GMT, Tim Prinzing wrote: >> JDK-8276674 : Malformed Javadoc inline tags in JDK source > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into JDK-8276674 > - Fixed @code and @link in some javadoc for JDK-8276674 Looks good. Thanks for renaming, per Pavel's suggestion. I can sponsor this. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v2]
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Brent Christian has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 32 commits:
- Merge branch 'master' into 8276447
- merge
- @SuppressWarnings(removal) in Windows CKey.java
- Add jls tag to runFinalization methods
- Update wording of Object.finalize, new paragraph is now bold
- update Object.finalize javadoc
- update Object.finalize JavaDoc and @deprecated tag
- Update Object.finalize deprecation message
- Indicate that runFinalizers does nothing if finalization is disabled or
removed
- Fix @since on @Deprecated for java.lang.Enum
- ... and 22 more: https://git.openjdk.java.net/jdk/compare/a363b7b9...e4986a48
-
Changes: https://git.openjdk.java.net/jdk/pull/6465/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6465=01
Stats: 194 lines in 62 files changed: 54 ins; 38 del; 102 mod
Patch: https://git.openjdk.java.net/jdk/pull/6465.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6465/head:pull/6465
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v3]
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Brent Christian has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 33 commits:
- Merge branch 'master' into 8276447
- Merge branch 'master' into 8276447
- merge
- @SuppressWarnings(removal) in Windows CKey.java
- Add jls tag to runFinalization methods
- Update wording of Object.finalize, new paragraph is now bold
- update Object.finalize javadoc
- update Object.finalize JavaDoc and @deprecated tag
- Update Object.finalize deprecation message
- Indicate that runFinalizers does nothing if finalization is disabled or
removed
- ... and 23 more: https://git.openjdk.java.net/jdk/compare/0dfb3a70...8cde0674
-
Changes: https://git.openjdk.java.net/jdk/pull/6465/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6465=02
Stats: 194 lines in 62 files changed: 54 ins; 38 del; 102 mod
Patch: https://git.openjdk.java.net/jdk/pull/6465.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6465/head:pull/6465
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks wrote: >> Pretty much what it says. The new option controls a static member in >> InstanceKlass that's consulted to determine whether the finalization >> machinery is activated for instances when a class is loaded. A new native >> method is added so that this state can be queried from Java. This is used to >> control whether a finalizer thread is created and to disable the `System` >> and `Runtime::runFinalization` methods. Includes tests for the above. >> >> Adding an option to disable finalization is part of [JEP >> 421](https://openjdk.java.net/jeps/421). > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Remove Finalizer.Holder class. Lib changes and tests look good - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Fri, 19 Nov 2021 18:06:39 GMT, Mandy Chung wrote:
>> Here are the code changes for the "Deprecate finalizers in the standard Java
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
>> review.
>>
>> This change makes the indicated deprecations, and updates the API spec for
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>>
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> src/java.base/share/classes/java/lang/Object.java line 481:
>
>> 479: * system resources or to perform other cleanup.
>> 480: *
>> 481: * When running in a Java virtual machine in which finalization
>> has been
>
> Disabling finalization is not part of the Java SE spec. This paragraph
> describes the implementation of HotSpot VM and I think it does not belong to
> this javadoc.
The coupling between this change and
[8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add command-line
option to disable finalization") is probably tighter than would be ideal. That
[CSR](https://bugs.openjdk.java.net/browse/JDK-8276773) allows for finalization
to be disabled in the SE Platform Spec and the JLS.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v3]
On Thu, 18 Nov 2021 21:15:11 GMT, Mandy Chung wrote: > When the finalization is disabled, perhaps jcmd GC.finalizer_info should just > be made as a nop in the VM. Would it be interesting (perhaps in a follow-up) for GC.finalizer_info to report that the given VM had finalization disabled? - PR: https://git.openjdk.java.net/jdk/pull/6442
RFR: JDK-8276447 Deprecate finalization-related methods for removal
Here are the code changes for the "Deprecate finalizers in the standard Java
API" portion of JEP 421 ("Deprecate Finalization for Removal") for code review.
This change makes the indicated deprecations, and updates the API spec for JEP
421. It also updates the relevant @SuppressWarning annotations.
The CSR has been approved.
An automated test build+test run passes cleanly (FWIW :D ).
-
Commit messages:
- merge
- @SuppressWarnings(removal) in Windows CKey.java
- Add jls tag to runFinalization methods
- Update wording of Object.finalize, new paragraph is now bold
- update Object.finalize javadoc
- update Object.finalize JavaDoc and @deprecated tag
- Update Object.finalize deprecation message
- Indicate that runFinalizers does nothing if finalization is disabled or
removed
- Fix @since on @Deprecated for java.lang.Enum
- Clarify conditions for removal of Object.finalize method
- ... and 21 more: https://git.openjdk.java.net/jdk/compare/89b125f4...eca95cb2
Changes: https://git.openjdk.java.net/jdk/pull/6465/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6465=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8276447
Stats: 194 lines in 62 files changed: 54 ins; 38 del; 102 mod
Patch: https://git.openjdk.java.net/jdk/pull/6465.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6465/head:pull/6465
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 20:18:17 GMT, Roger Riggs wrote: >> You have a point. >> >> BUT, at least it's a working example and not some pseudo code. We do want to >> move to working example code long term, don't we? >> >> When I see , I'm just wondering what those <> type operators are >> good for here... > > A cleaner can/should be shared within some scope and purpose, in this case > the example class . > Each cleaner has a dedicated Thread so its not a lightweight object and > should not be proliferated. > (Loom may be able to use Virtual Threads). > > The reasons to not share are a Cleaner are based on possible interference > between the cleanup callbacks. > If they share a thread and are non-trivial, it might slow other cleaners. If > the cleaner is created and shared > for a particular purpose it will share the thread resource and still be > efficient. The new example Cleaner instance _is_ shared, though on a pretty small scale (just among instances of CleaningExample). A demonstration of larger scale sharing of a Cleaner instance would be out of scope for this example. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 17:15:12 GMT, Mandy Chung wrote: >> I don't believe so. `no reference to the instance being cleaned` is the >> essential part (to me). > > This is what I suggested and makes it clear that *must hold no reference to > the instance being cleaned*. Maybe you didn't notice it's still there? > > > *// State class captures information necessary for cleanup. > *// It must hold no reference to the instance being cleaned > *// and therefore it is a static inner class in this CleaningExample > ``` I like this suggestion - calling out that the static-ness of the State class is due to the requirement to not hold any references. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8271302: Regex Test Refresh [v2]
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves wrote: >> 8271302: Regex Test Refresh > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Couple of fixes Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves wrote:
> 8271302: Regex Test Refresh
Changes requested by bchristi (Reviewer).
In the JBS issue, it looks like the Description was put in the Environment. :)
test/jdk/java/util/regex/RegExTest.java line 291:
> 289:
> 290: int resultStart1 = mr.start();
> 291: assertEquals(matcherStart1, resultStart1, "equal matchers have
> equal start indices");
Should the message be that they *don't* have equal start indices ?
test/jdk/java/util/regex/RegExTest.java line 2362:
> 2360:
> 2361: { "test\ud834\uddc0", "test\ud834\uddc0",
> "m", true },
> 2362: //{ "test\ud834\uddbc\ud834\udd6f", "test\ud834\uddc0",
> "m", true }, //problem
Should an issue be filed for these //problems ?
test/jdk/java/util/regex/RegExTest.java line 3952:
> 3950:
> 3951: m = Pattern.compile("\\H").matcher(matcherSubstring);
> 3952: assertTrue(m.find() || ng.equals(m.group()));
Should this be:
`assertTrue(m.find() && ng.equals(m.group()));`
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8266936: Add a finalization JFR event [v4]
On Tue, 27 Jul 2021 15:14:29 GMT, Markus Grönlund wrote: >> Greetings, >> >> Object.finalize() was deprecated in JDK9. There is an ongoing effort to >> replace and mitigate Object.finalize() uses in the JDK libraries; please see >> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. >> >> We also like to assist users in replacing and mitigating uses in non-JDK >> code. >> >> Hence, this changeset adds a periodic JFR event to help identify which >> classes are overriding Object.finalize(). >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > remove build directive Hi, Bernd The JFR event can complement static bytecode analysis, reporting which classes with finalizers actually get loaded at runtime. I agree that it would be useful to add a count for finalizers that get run, either as part of this PR, or as a follow-up. I would hesitate to add timing, though. It's important to keep the overhead of this event low, and finalize() methods should already show up in current profilers. Thanks, -Brent > _Mailing list message from [Bernd Eckenfels](mailto:e...@zusammenkunft.net) > on [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_ > > Hello, > > I know I am a bit late, but just wanted to mention, that since finding > finalizers with Bytecode analysis is doable (and probably easier to deal with > such scan reports), I don?t see much value in a JFR event, especially > considering it even has native code executed. (Not so sure about dynamically > loaded classes, would the event content help to identify sources?) > > Having said that, this event would be more useful from a runtime perspective > if It would actually record execution counts and time per class. Then one can > concentrate on the worst offenders, first and even use it for runtime > monitoring. Is there already a finalizer profiler? > > Gruss > Bernd - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event
On Thu, 8 Jul 2021 19:47:26 GMT, Markus Grönlund wrote: > Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We would also like to assist users in replacing and mitigating uses in > non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus src/hotspot/share/jfr/metadata/metadata.xml line 1084: > 1082: > 1083: > 1084:thread="false" startTime="false" period="endChunk"> Would it make sense for this event to be under the "Java Virtual Machine, GC" category? - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: [jdk17] RFR: 8268826: Cleanup Override in Context-Specific Deserialization Filters [v7]
On Wed, 7 Jul 2021 14:55:18 GMT, Roger Riggs wrote: >> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory >> property. >> Fix description in the example of a filter allowing platform classes. >> Suppress some warnings about use of SecurityManager in tests. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Update SerialFactoryExample.PredicateFilter to match the OIF implementation > Test cleanup; enable logging of serialization on the command line (remove > static) > Remove obsolete comment about jdk.serialFilter > Fix typo in StaticProperty.java Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/85
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters [v6]
On Fri, 25 Jun 2021 14:54:45 GMT, Roger Riggs wrote:
>> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory
>> property.
>> Fix description in the example of a filter allowing platform classes.
>> Suppress some warnings about use of SecurityManager in tests.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update logging of faults in jdk.serialFilterFactory to log only the
> exception message
> Simplify the logging.properties to only the needed settings
Marked as reviewed by bchristi (Reviewer).
test/jdk/java/io/Serializable/serialFilter/SerialFilterFunctionTest.java line
49:
> 47: // Enable logging
> 48: System.setProperty("java.util.logging.config.file",
> 49: System.getProperty("test.src", ".") +
> "/logging.properties");
Is `System.setProperty()` needed if it's already being set with -D ?
-
PR: https://git.openjdk.java.net/jdk17/pull/85
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters [v4]
On Wed, 23 Jun 2021 19:12:06 GMT, Roger Riggs wrote: >> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory >> property. >> Fix description in the example of a filter allowing platform classes. >> Suppress some warnings about use of SecurityManager in tests. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Improve exception cases and messages when the jdk.serialFilterFactory > is misconfigured and test those faults. > Fix typo in java.security-extra-factory test config Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/85
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters [v2]
On Mon, 21 Jun 2021 17:15:03 GMT, Roger Riggs wrote: >> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory >> property. >> Fix description in the example of a filter allowing platform classes. >> Suppress some warnings about use of SecurityManager in tests. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove jdk.serialFilterFactory "OVERRIDE" special handling. > Correct description of the example of allowing platform or bootstrap > classes in OIF. > Cleanup of logging of filter tracing. Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/85
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters
On Wed, 16 Jun 2021 20:22:17 GMT, Roger Riggs wrote: > Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory > property. > Fix description in the example of a filter allowing platform classes. > Suppress some warnings about use of SecurityManager in tests. OVERRIDE is also mentioned in `src/java.base/share/conf/security/java.security b/src/java.base/share/conf/security/java.security` - Changes requested by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/85
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v15]
On Tue, 8 Jun 2021 14:26:43 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >> https://openjdk.java.net/jeps/415. >> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are >> extended with additional >> configuration mechanisms and filter utilities. >> >> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and >> `ObjectInputStream`: >> >> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Clarify exceptions that occur if initializing the filter factory from > jdk.serialFilterFactory fails Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8267569: java.io.File.equals contains misleading Javadoc [v3]
On Tue, 1 Jun 2021 17:43:42 GMT, Brian Burkhalter wrote: >> Modify the specification of `java.io.File.equals` to clarify that equality >> is based only on a comparison of abstract pathnames as opposed to the file >> system objects that the `File`s represent. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8267569: Change @implNote to @apiNote Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4232
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v12]
On Wed, 26 May 2021 22:11:54 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> 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 13 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8264859-context-filter-factory
> - Added test for rejectUndecidedClass array cases
>Added test for preventing disabling filter factory
>Test cleanup
> - Editorial updates to review comments.
>Simplify the builtin filter factory implementation.
>Add atomic update to setting the filter factory.
>Clarify the description of OIS.setObjectInputFilter.
>Cleanup of the example code.
> - Editorial updates
>Updated java.security properties to include jdk.serialFilterFactory
>Added test cases to SerialFilterFactoryTest for java.security properties
> and
>enabling of the SecurityManager with existing policy permission files
>Corrected a test that OIS.setObjectInputFilter could not be called twice.
>Removed a Factory test that was not intended to be committed
> - Moved utility filter methods to be static on ObjectInputFilter
>Rearranged the class javadoc of OIF to describe the parts of
>deserialization filtering, filters, composite filters, and the filter
> factory.
>And other review comment updates...
> - Refactored tests for utility functions to SerialFilterFunctionTest.java
>Deleted confused Config.allowMaxLimits() method
>Updated example to match move of methods to Config
>Added test of restriction on setting the filterfactory after a OIS has
> been created
>Additional Editorial updates
> - Move merge and rejectUndecidedClass methods to OIF.Config
>As default methods on OIF, their implementations were not concrete and not
> trustable
> - Review suggestions included;
>Added @implSpec for default methods in OIF;
>Added restriction that the filter factory cannot be set after an
> ObjectInputStream has been created and applied the current filter factory
> - Editorial javadoc updated based on review comments.
>Clarified behavior of rejectUndecidedClass method.
>Example test added to check status returned from file.
> - Editorial updates to review comments
>Add filter tracing support
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/30e4a509...0930f0f8
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/io/ObjectInputFilter.java line 137:
> 135: * {@linkplain #allowFilter(Predicate, Status) allow} or
> 136: * {@linkplain #rejectFilter(Predicate, Status) reject} classes.
> 137: *.
Extra '.' on this line
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v10]
On Wed, 26 May 2021 16:48:53 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial updates to review comments.
> Simplify the builtin filter factory implementation.
> Add atomic update to setting the filter factory.
> Clarify the description of OIS.setObjectInputFilter.
> Cleanup of the example code.
test/jdk/java/io/Serializable/serialFilter/SerialFilterFactoryTest.java line
328:
> 326: public void current(ObjectInputFilter current) {
> 327: this.current = current;
> 328: }
Is this current() used anywhere ?
test/jdk/java/io/Serializable/serialFilter/SerialFilterFactoryTest.java line
332:
> 330: public void next(ObjectInputFilter next) {
> 331: this. next = next;
> 332: }
Is this next() used anywhere ?
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v9]
On Tue, 25 May 2021 21:45:33 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial updates
> Updated java.security properties to include jdk.serialFilterFactory
> Added test cases to SerialFilterFactoryTest for java.security properties and
> enabling of the SecurityManager with existing policy permission files
> Corrected a test that OIS.setObjectInputFilter could not be called twice.
> Removed a Factory test that was not intended to be committed
src/java.base/share/classes/java/io/ObjectInputFilter.java line 513:
> 511: * the static JVM-wide filter, or to create a filter from a pattern
> string.
> 512: * The static filter factory and the static filter apply to the
> whole Java runtime,
> 513: * or "JVM-wide", there is only one of each, for a complete
> description of
Suggest new sentence after "_...there is only one of each. For a complete..._"
src/java.base/share/classes/java/io/ObjectInputFilter.java line 551:
> 549: final class Config {
> 550: /**
> 551: * Lock object for filter and filter factory.
The lock is not used for the filter factory, is it?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 768:
> 766: * This package private method is *only* called by {@link
> ObjectInputStream#ObjectInputStream()}
> 767: * and {@link ObjectInputStream#ObjectInputStream(InputStream)}.
> 768: * {@link ObjectInputFilter.Config#serialFilterFactory} does the
> enforcement.
Is this still true about the enforcement?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1251:
> 1249: * Returns REJECTED if either of the filters returns
> REJECTED,
> 1250: * otherwise, ALLOWED if either of the filters returns
> ALLOWED.
> 1251: * otherwise, returns {@code UNDECIDED}.
Capitalize "Otherwise"
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1256:
> 1254: * @return REJECTED if either of the filters returns
> REJECTED,
> 1255: * otherwise, ALLOWED if either of the filters
> returns ALLOWED.
> 1256: * otherwise, returns {@code UNDECIDED}.
Otherwise
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v8]
On Tue, 25 May 2021 15:46:37 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Moved utility filter methods to be static on ObjectInputFilter
>Rearranged the class javadoc of OIF to describe the parts of
>deserialization filtering, filters, composite filters, and the filter
> factory.
>And other review comment updates...
> - Refactored tests for utility functions to SerialFilterFunctionTest.java
>Deleted confused Config.allowMaxLimits() method
>Updated example to match move of methods to Config
>Added test of restriction on setting the filterfactory after a OIS has
> been created
>Additional Editorial updates
More suggestions for ObjectInputFilter.java. I hope they're helpful.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 128:
> 126: * provides the {@linkplain Config#getSerialFilter static JVM-wide
> filter} when invoked from the
> 127: * {@linkplain ObjectInputStream#ObjectInputStream(InputStream)
> ObjectInputStream constructors}
> 128: * and replaces the static filter and when invoked from
"...replaces the static filter ~~and~~ when invoked..."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 137:
> 135: * or to {@linkplain #allowFilter(Predicate, Status) allow} or
> 136: * {@linkplain #rejectFilter(Predicate, Status) reject} classes based on
> a
> 137: * {@linkplain Predicate predicate of a class}.
Maybe a new sentence for the Predicate-based filters? e.g. "Filters can be
created from a pattern string. Or they can allow or reject classes based on a
Predicate."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 205:
> 203: * // Returns a composite filter of the static JVM-wide filter, a
> thread-specific filter,
> 204: * // and the stream-specific filter.
> 205: * public ObjectInputFilter apply(ObjectInputFilter curr,
> ObjectInputFilter next) {
`@Override` on `apply` ?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 211:
> 209: * if (filter != null) {
> 210: * // Prepend a filter to assert that all classes have
> been Allowed or Rejected
> 211: * filter =
> ObjectInputFilter.Config.rejectUndecidedClass(filter);
~~Config~~ throughout the example
src/java.base/share/classes/java/io/ObjectInputFilter.java line 301:
> 299: * on the class is {@code true}.
> 300: * The filter returns {@code ALLOWED} or the {@code otherStatus}
> based on the predicate
> 301: * of the {@code non-null} class and {@code UNDECIDED} if the class
> is {@code null}.
Can this sentence ("_The filter returns...class is null_") be removed? IMO it
doesn't add to what's covered in the rest of the method doc.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 334:
> 332: * on the class is {@code true}.
> 333: * The filter returns {@code REJECTED} or the {@code otherStatus}
> based on the predicate
> 334: * of the {@code non-null} class and {@code UNDECIDED} if the class
> is {@code null}.
Same - can this sentence ("The filter returns...class is null") be removed?
-
Changes requested by bchristi (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]
On Mon, 24 May 2021 21:57:50 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move merge and rejectUndecidedClass methods to OIF.Config
> As default methods on OIF, their implementations were not concrete and not
> trustable
src/java.base/share/classes/java/io/ObjectInputFilter.java line 177:
> 175: * // Initially this would be the static JVM-wide filter
> passed from the OIS constructor
> 176: * // Append the filter to reject all UNDECIDED results
> 177: * filter = next.merge(filter).rejectUndecidedClass();
Update for merge() now being class method
src/java.base/share/classes/java/io/ObjectInputFilter.java line 866:
> 864: /**
> 865: * Returns a filter that merges the status of a filter and
> another filter.
> 866: * If the other filter is {@code null}, the filter is returned.
Now that this method is static, this sentence could be further clarified with
some markup, IMO:
"If `{@code anotherFilter}` is `{@code null}`, `{@code filter}` is returned."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 874:
> 872: * Invoke {@code filter} on the {@code FilterInfo} to
> get its {@code status};
> 873: * Return {@code REJECTED} if the {@code status} is
> {@code REJECTED};
> 874: * Invoke the {@code otherFilter} to get the {@code
> otherStatus};
"the `otherFilter`" -> "`anotherFilter`"
src/java.base/share/classes/java/io/ObjectInputFilter.java line 892:
> 890:
> 891: /**
> 892: * Returns a filter that invokes a filter and maps {@code
> UNDECIDED} to {@code REJECTED}
"...that invokes _the given_ filter..." ?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 895:
> 893: * for classes, with some exceptions, and otherwise returns the
> status.
> 894: * The filter returned checks that classes not {@code ALLOWED}
> and not {@code REJECTED} by the filter
> 895: * are {@code REJECTED}, if the class is an array and the base
> component type is not allowed,
Could/should this be simplified to, "...checks that classes not ALLOWED by the
filter are REJECTED."?
Also, I would add something like, "...,_including_ if the class is..." or
"...,_even_ if the class is..."; otherwise it sounds a bit like this _only_
applies to arrays.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1422:
> 1420: * {@linkplain ObjectInputStream#ObjectInputStream(InputStream)
> ObjectInputStream constructors}.
> 1421: * When invoked from {@link
> ObjectInputStream#setObjectInputFilter(ObjectInputFilter)
> 1422: * to set the stream-specific filter} the requested filter
> replaces the static serial filter,
"When invoked _from to_ set the..."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1477:
> 1475:
> 1476: /**
> 1477: * Returns the class name name of this builtin
> deserialization filter factory.
name name
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v6]
On Mon, 24 May 2021 17:24:22 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review suggestions included;
> Added @implSpec for default methods in OIF;
> Added restriction that the filter factory cannot be set after an
> ObjectInputStream has been created and applied the current filter factory
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1222:
> 1220: /**
> 1221: * Apply the predicate to the class being deserialized, if
> the class is non-null
> 1222: * and if it returns {@code true}, return the requested
> status. Otherwise, return UNDECIDED.
Isn't `ifFalseStatus` returned in the "otherwise" case, not (necessarily)
`UNDECIDED` ?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1236:
> 1234: public String toString() {
> 1235: return "predicate(" + predicate + ")";
> 1236: }
Would it also be useful to also print `ifTrueStatus` and `ifFalseStatus` ? Not
sure, given this is an internal class...
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1283:
> 1281: * Returns REJECTED if either of the filters returns
> REJECTED,
> 1282: * and ALLOWED if either of the filters returns ALLOWED.
> 1283: * Returns {@code UNDECIDED} if either filter returns
> {@code UNDECIDED}.
The description of when `UNDECIDED` is returned should match `@return`.
Alternately, have minimal documentation here, and refer to the public merge()
method, which this implements.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v4]
On Fri, 21 May 2021 17:47:53 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial updates to review comments
> Add filter tracing support
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/io/ObjectInputFilter.java line 84:
> 82: *
> 83: * When a filter is set on an {@link ObjectInputStream}, the {@link
> #checkInput checkInput(FilterInfo)}
> 84: * method is called to validate classes, the length of each array,
This sounds a bit like checkInput() is called at the time that the filter is
set. I'd recommend something like, "If a filter is set on an
ObjectInputStream, the checkInput() method is called during deserialization to
validate..."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 123:
> 121: * the depth, number of references, and stream size and return a status
> 122: * that reflects only one or only some of the values.
> 123: * This allows a filter to specific about the choice it is reporting and
...to _**be**_ specific ?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 386:
> 384: * {@link ObjectInputStream#setObjectInputFilter(ObjectInputFilter)
> ObjectInputStream.setObjectInputFilter}.
> 385: * If {@code ObjectInputStream.setObjectInputFilter} is called, the
> filter factory is called a second time
> 386: * with the initial filter returned from the first call and the
> requested new filter.
Maybe shorten to, "with the stream's initial filter, and the requested new
filter."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 404:
> 402: * The JVM-wide filter is configured during the initialization of the
> 403: * {@code ObjectInputFilter.Config} class.
> 404: * For example, by calling {@link #getSerialFilter()
> Config.getSerialFilter}.
Can the "For example..." sentence be removed?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 423:
> 421: * The class must be public, must have a public zero-argument
> constructor, implement the
> 422: * {@link BinaryOperator {@literal
> BinaryOperator}} interface, provide its implementation and
> 423: * be accessible via the {@linkplain
> ClassLoader#getSystemClassLoader() the application class loader}.
two "the"s
src/java.base/share/classes/java/io/ObjectInputFilter.java line 1258:
> 1256: /**
> 1257: * Apply the filter and return the status if not UNDECIDED
> and checking a class.
> 1258: * Make an exception for Primitive classes that are
> implicitly allowed by the pattern based filte.
filte -> filter
src/java.base/share/classes/java/io/ObjectInputStream.java line 200:
> 198: * and every object read from the stream can be checked.
> 199: * The {@linkplain #ObjectInputStream() ObjectInputStream constructors}
> invoke the filter factory
> 200: * to select the initial filter and it is updated by {@link
> #setObjectInputFilter}.
In ObjectInputFilter, the `"The deserialization filter for a stream is
determined in one of the following ways"` section has a good description of the
various pieces and how they work together to determine a stream's filter. I
would consider leaning on that to provide the details, and have a shorter,
high-level description here. For instance:
"Each stream has an optional deserialization filter to check the classes and
resource limits during deserialization. The filter can be set for all streams
JVM-wide, or customized on a per-stream basis. See ``
for details on how the filter for a stream is determined.
ObjectInputFilter describes how to use filters and..."
src/java.base/share/classes/java/io/ObjectInputStream.java line 370:
> 368: *
> 369: * The serialization filter is initialized to the filter returned
> 370: * by invoking the {@link Config#getSerialFilterFactory()} with
> {@code null} for the current filter
`linkplain` to "JVM-wide filter factory" ?
src/java.base/share/classes/java/io/ObjectInputStream.java line 371:
> 369: * The serialization filter is initialized to the filter returned
> 370: * by invoking the {@link Config#getSerialFilterFactory()} with
> {@code null} for the current filter
> 371: * and {@linkplain Config#getSerialFilter() static JVM-wide filter}
> for the
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify factory interface to BinaryOperator and cleanup
> the example
src/java.base/share/classes/java/io/ObjectInputFilter.java line 273:
> 271: * {@link ObjectInputFilter.Status#REJECTED}, if the filter
> is checking a class
> 272: * and the filter returns {@link
> ObjectInputFilter.Status#UNDECIDED},
> 273: * Otherwise, return the status
"Otherwise, return the status of this filter" ?
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >> https://openjdk.java.net/jeps/415. >> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are >> extended with additional >> configuration mechanisms and filter utilities. >> >> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and >> `ObjectInputStream`: >> >> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify factory interface to BinaryOperator and cleanup > the example src/java.base/share/classes/java/io/ObjectInputStream.java line 193: > 191: * the state. > 192: * > 193: * The deserialization filter for a stream is determined in one of the > following ways: Should this line start with a `` ? - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8266936: Add a finalization JFR event [v2]
> Please review this enhancement to add a new JFR event, generated whenever a > finalizer is run. > (The makeup is similar to the Deserialization event, > [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) > > The event's only datum (beyond those common to all jfr events) is the class > of the object that was finalized. > > The Category for the event: > `"Java Virtual Machine" / "GC" / "Finalization"` > is what made sense to me, even though the event is generated from library > code. > > Along with the new regtest, I added a run mode to the basic finalizer test to > enable jfr. > Automated testing looks good so far. > > Thanks, > -Brent Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Test flag should be volatile - Changes: - all: https://git.openjdk.java.net/jdk/pull/4101/files - new: https://git.openjdk.java.net/jdk/pull/4101/files/200268ab..e0ef383b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4101=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4101=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4101.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4101/head:pull/4101 PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event
On Tue, 18 May 2021 21:40:57 GMT, Istvan Neuwirth
wrote:
>> Please review this enhancement to add a new JFR event, generated whenever a
>> finalizer is run.
>> (The makeup is similar to the Deserialization event,
>> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).)
>>
>> The event's only datum (beyond those common to all jfr events) is the class
>> of the object that was finalized.
>>
>> The Category for the event:
>> `"Java Virtual Machine" / "GC" / "Finalization"`
>> is what made sense to me, even though the event is generated from library
>> code.
>>
>> Along with the new regtest, I added a run mode to the basic finalizer test
>> to enable jfr.
>> Automated testing looks good so far.
>>
>> Thanks,
>> -Brent
>
> test/jdk/jdk/jfr/event/gc/finalization/TestFinalizerEvent.java line 48:
>
>> 46:
>> 47: public class TestFinalizerEvent {
>> 48: static boolean finalizerRun = false;
>
> Should not this be a `volatile` field? (Or alternatively, using
> `AtomicBoolean`). As the `finalize` is invoked on the finalizer thread, the
> other thread executing the test might not see the change.
Good idea, thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/4101
RFR: 8266936: Add a finalization JFR event
Please review this enhancement to add a new JFR event, generated whenever a finalizer is run. (The makeup is similar to the Deserialization event, [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) The event's only datum (beyond those common to all jfr events) is the class of the object that was finalized. The Category for the event: `"Java Virtual Machine" / "GC" / "Finalization"` is what made sense to me, even though the event is generated from library code. Along with the new regtest, I added a run mode to the basic finalizer test to enable jfr. Automated testing looks good so far. Thanks, -Brent - Commit messages: - Fix jcheck some more - Fix jcheck - Merge branch 'master' into 8266936 - Add jfr mode to test/jdk/java/lang/ref/FinalizeOverride.java - Fix spacing in new regtest - Remove extraneous comments from new regtest - Removed unintentional changes from FinalizeOverride.java - Actually check in new regtest - Update TestActiveSettingEvent.java - Update TestDefaultConfigurations.java and .jfc files - ... and 2 more: https://git.openjdk.java.net/jdk/compare/e6705c0e...200268ab Changes: https://git.openjdk.java.net/jdk/pull/4101/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4101=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266936 Stats: 191 lines in 11 files changed: 190 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4101.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4101/head:pull/4101 PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8264048: Fix caching in Jar URL connections when an entry is missing [v2]
On Wed, 31 Mar 2021 12:47:50 GMT, Aleksei Efimov wrote: >> Current fix tries to tackle an issue with URL connection referencing >> non-existing Jar file entries: >> If an entry that doesn't exist is specified in an URL connection the >> underlying Jar file is still cached even if an exception is thrown after >> that. Such behavior prevents the caller, for instance, a `URLClassLoader`, >> from closing a Jar file. >> >> The proposed fix checks if entry exists before caching a Jar file (only for >> cases with enabled caching): >> - If entry exists - jar file is cached if it wasn't cached before >> - If entry doesn't exist and jar file wasn't cached before - jar file is >> closed and exception is thrown >> - If entry doesn't exist and jar file was cached before - jar file is kept >> cached and exception is thrown >> >> >> The following tests have been used to verify the fix: >> - New regression tests >> - ``:jdk_core:`` tests >> - `api/java_util`,`api/java_net` JCK tests > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8264048: Remove old version of RemoveJar test Hi, Aleksei. The change looks good. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3263
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v3]
On Tue, 23 Mar 2021 01:03:55 GMT, Kim Barrett wrote: >> Please review this change to java.util.Timer, replacing the use of >> deprecated finalization-based cleanup with use of java.lang.ref.Cleaner. >> >> In addition, Timer.cancel now cancels any later execution of the the no >> longer relevant cleanup. >> >> Testing: >> mach5 tier1 >> New AutoStop test verifies the specified cleanup behavior. >> (There are existing tests involving Timer.cancel.) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > make ThreadReaper constructor non-public Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v2]
On Mon, 22 Mar 2021 07:19:28 GMT, Kim Barrett wrote: >> Please review this change to java.util.Timer, replacing the use of >> deprecated finalization-based cleanup with use of java.lang.ref.Cleaner. >> >> In addition, Timer.cancel now cancels any later execution of the the no >> longer relevant cleanup. >> >> Testing: >> mach5 tier1 >> New AutoStop test verifies the specified cleanup behavior. >> (There are existing tests involving Timer.cancel.) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > dholmes review The change looks good to me (though making the ThreadReaper ctor non-public would be nice). - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8262351: Extra '0' in java.util.Formatter for '%012a' conversion with a sign character [v2]
On Wed, 10 Mar 2021 02:31:28 GMT, Ian Graves wrote: >> This fixes a zero-adding issue observed in the hex float conversion. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Updating Formatter copyright date Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2881
Re: RFR: 8262351: Extra '0' in java.util.Formatter for '%012a' conversion with a sign character
On Mon, 8 Mar 2021 21:25:32 GMT, Ian Graves wrote: > This fixes a zero-adding issue observed in the hex float conversion. This all looks fine - just update the copyright year in Formatter.java, please. In my personal opinion, this behavior change does not rise to the level of needing a CSR, but since it is long-standing behavior, you might double-check with Joe. - Changes requested by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2881
Re: RFR: JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException [v5]
On Sat, 6 Mar 2021 01:35:28 GMT, Craig Andrews
wrote:
>> `java.net.URLClassLoader.getResource` can throw an undocumented
>> `IllegalArgumentException`.
>>
>> According to the javadoc for the `getResource` and `findResource` methods,
>> neither should be throwing `IllegalArgumentException` - they should return
>> null if the resource can't be resolved.
>>
>> Quoting the javadoc for
>> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>>> Returns:
>>> a URL for the resource, or null if the resource could not be found, or
>>> if the loader is closed.
>>
>> And quoting the javadoc for
>> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>>> Returns:
>>> URL object for reading the resource; null if the resource could not be
>>> found, a URL could not be constructed to locate the resource, the resource
>>> is in a package that is not opened unconditionally, or access to the
>>> resource is denied by the security manager.
>>
>> Neither mentions throwing `IllegalArgumentException` and both are clear that
>> when URL can't be constructed, `null` should be returned.
>>
>> Here's a stack trace:
>> java.lang.IllegalArgumentException: name
>> at
>> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
>> at
>> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
>> at java.base/java.security.AccessController.doPrivileged(Native
>> Method)
>> at
>> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>>
>> Looking at
>> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>>
>> A similar issue exists at
>> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>
> Craig Andrews has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> JDK-8262277: java.net.URLClassLoader.getResource throws undocumented
> IllegalArgumentException
Looks good, thanks!
-
Marked as reviewed by bchristi (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException [v4]
On Fri, 5 Mar 2021 16:02:49 GMT, Craig Andrews
wrote:
>> `java.net.URLClassLoader.getResource` can throw an undocumented
>> `IllegalArgumentException`.
>>
>> According to the javadoc for the `getResource` and `findResource` methods,
>> neither should be throwing `IllegalArgumentException` - they should return
>> null if the resource can't be resolved.
>>
>> Quoting the javadoc for
>> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>>> Returns:
>>> a URL for the resource, or null if the resource could not be found, or
>>> if the loader is closed.
>>
>> And quoting the javadoc for
>> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>>> Returns:
>>> URL object for reading the resource; null if the resource could not be
>>> found, a URL could not be constructed to locate the resource, the resource
>>> is in a package that is not opened unconditionally, or access to the
>>> resource is denied by the security manager.
>>
>> Neither mentions throwing `IllegalArgumentException` and both are clear that
>> when URL can't be constructed, `null` should be returned.
>>
>> Here's a stack trace:
>> java.lang.IllegalArgumentException: name
>> at
>> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
>> at
>> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
>> at java.base/java.security.AccessController.doPrivileged(Native
>> Method)
>> at
>> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>>
>> Looking at
>> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>>
>> A similar issue exists at
>> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>
> Craig Andrews has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> JDK-8262277: java.net.URLClassLoader.getResource throws undocumented
> IllegalArgumentException
Changes requested by bchristi (Reviewer).
test/jdk/java/net/URLClassLoader/FindResourceDoesNotThrowException.java line 27:
> 25: * @test
> 26: * @bug 8262277
> 27: * @summary Test to see if URLClassLoader.getResource and
> URLClassLoader.getResources
The summary mentions the get* methods, but the test calls the find* methods. I
think it would be good to test all four methods (getResource, getResources,
findResource, findResources), and update the summary e.g. "Test if
URLClassLoader throws IllegalArgumentException when getting or finding
resources."
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException [v2]
On 3/4/21 1:16 PM, Craig Andrews wrote: @bchristi-git has indicated that a [compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request is needed for this pull request. @candrews please create a CSR request and add link to it in [JDK-8262277](https://bugs.openjdk.java.net/browse/JDK-8262277). This pull request cannot be integrated until the CSR request is approved. I'm trying to figure out how to create a CSR and I'm not having much luck. According to the [CSR FAQs](https://wiki.openjdk.java.net/display/csr/CSR+FAQs): Q: How do I create a CSR ? A: Do not directly create a CSR from the Create Menu. JIRA will let you do this right up until the moment you try to save it and find your typing was in vain. Instead you should go to the target bug, select "More", and from the drop down menu select "Create CSR". This is required to properly associate the CSR with the main bug, just as is done for backports. However, I don't have an account on https://bugs.openjdk.java.net so I can't do as instructed. How do I create the CSR? OK, yeah - I can create the CSR. If you can craft some content for the Summary, Problem, Solution, and maybe Compatibility Risk Description (the risk itself is Low), I can take it from there. A recent CSR of a somewhat similar fix-the-implementation-to-match-the-spec flavor might be: https://bugs.openjdk.java.net/browse/JDK-8255997 (though the change there is to *start* throwing an exception :D ). Anyway, perhaps that might helpful as a reference. -Brent
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Wed, 3 Mar 2021 18:10:25 GMT, Brent Christian wrote: >> This seems to be a long standing bug, maybe goes all the way back to JDK >> 1.2. Are you planning to add a regression test? > > Hi, Craig > The commented-out lines should be removed from the change. > > As Alan said, a regression test will be needed. At minimum, it should test a > method that returns a URL, as well as a method that returns an > Enumeration (which can also lead to an IAE, as I noted in the bug > report). > > Also, though the compatibility risk is low, it would be good to include a CSR > for this change to long-standing behavior. Also, the copyright year should be updated: 2019 -> 2021 - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Wed, 3 Mar 2021 14:28:00 GMT, Alan Bateman wrote:
>> _NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
>> review ID : 9069151)
>>
>> `java.net.URLClassLoader.getResource` can throw an undocumented
>> `IllegalArgumentException`.
>>
>> According to the javadoc for the `getResource` and `findResource` methods,
>> neither should be throwing `IllegalArgumentException` - they should return
>> null if the resource can't be resolved.
>>
>> Quoting the javadoc for
>> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>>> Returns:
>>> a URL for the resource, or null if the resource could not be found, or
>>> if the loader is closed.
>>
>> And quoting the javadoc for
>> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>>> Returns:
>>> URL object for reading the resource; null if the resource could not be
>>> found, a URL could not be constructed to locate the resource, the resource
>>> is in a package that is not opened unconditionally, or access to the
>>> resource is denied by the security manager.
>>
>> Neither mentions throwing `IllegalArgumentException` and both are clear that
>> when URL can't be constructed, `null` should be returned.
>>
>> Here's a stack trace:
>> java.lang.IllegalArgumentException: name
>> at
>> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
>> at
>> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
>> at java.base/java.security.AccessController.doPrivileged(Native
>> Method)
>> at
>> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>>
>> Looking at
>> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>>
>> A similar issue exists at
>> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>
> This seems to be a long standing bug, maybe goes all the way back to JDK 1.2.
> Are you planning to add a regression test?
Hi, Craig
The commented-out lines should be removed from the change.
As Alan said, a regression test will be needed. At minimum, it should test a
method that returns a URL, as well as a method that returns an Enumeration
(which can also lead to an IAE, as I noted in the bug report).
Also, though the compatibility risk is low, it would be good to include a CSR
for this change to long-standing behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: Integrated: JDK-8261003 : Bad Copyright header format after JDK-8183372
On Tue, 2 Feb 2021 22:08:33 GMT, Mahendra Chhipa wrote: > JDK-8261003 : Bad Copyright header format after JDK-8183372 Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2365
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v12]
On Tue, 2 Feb 2021 21:00:00 GMT, Mahendra Chhipa wrote: >> https://bugs.openjdk.java.net/browse/JDK-8183372 > > Mahendra Chhipa has updated the pull request incrementally with one > additional commit since the last revision: > > Generate the source files in JTWork directory. Looks good. Thanks for the changes. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Wed, 27 Jan 2021 23:03:55 GMT, Mahendra Chhipa wrote: >> test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 126: >> >>> 124: Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); >>> 125: FileUtils.deleteFileTreeWithRetry(pkg1Dir); >>> 126: } >> >> I'm not convinced the `@AfterClass` method is necessary. Pre-cleanup is >> already done on LL94-95. And occasionally, test-generated artifacts are >> helpful in post-mortem analysis. > > To keep the source code repo clean from temporary generated files, these > files are removed after test. Test logs have the information about the > generated files for the Postmortem analysis. Test files shouldn't be written into the repository (repos are sometimes tested on read-only filesystems). Files should be written under JTwork/scratch/. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
On Mon, 1 Feb 2021 23:43:25 GMT, Brent Christian wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > Changes requested by bchristi (Reviewer). (I think my comments may no have been visible because I didn't "submit" my review. Hopefully they are now.) - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v10]
On Mon, 1 Feb 2021 22:23:02 GMT, Mahendra Chhipa
wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> throwing the specific exceptions.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 159:
> 157: private void match(String actual, String expected) {
> 158: System.out.println("actual:" + actual + "expected:" + expected);
> 159: assert((actual == null && expected == null) ||
> actual.equals(expected.trim()));
I don't think this check is done, because assertions aren't enabled. This
should be changed to explicitly throw an exception.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
On Wed, 27 Jan 2021 22:44:00 GMT, Mahendra Chhipa
wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Implemented the review comments.
Changes requested by bchristi (Reviewer).
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 157:
> 155:
> 156: private void match(final String actual, final String expected) {
> 157: System.out.println("actual:" + actual + "expected:" + expected);
It would be good to insert a space before `expected:`
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 158:
> 156: private void match(final String actual, final String expected) {
> 157: System.out.println("actual:" + actual + "expected:" + expected);
> 158: assert ((actual == null && expected == null) ||
> actual.equals(expected.trim()));
I think we need to figure out where the extra space is coming from, if the test
worked before, but now fails without adding the `trim()`.
Also, I think it would be better to throw an exception instead of using
`assert`, so the test works with or without assertions being enabled.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa
wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Implemented the review comments.
I like keeping the changes within the existing .java files, thanks. I have a
few more suggestions.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 76:
> 74: * @modules jdk.compiler
> 75: * @build jdk.test.lib.process.* EnclosingClassTest
> 76: * @run testng/othervm EnclosingClassTest
The test should still be run with -esa -ea
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 88:
> 86: @BeforeClass
> 87: public void createEnclosingClasses() throws Throwable {
> 88: Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC);
The 'enclosingPath' Path could be the constant. Then ENCLOSING_CLASS_SRC is
not needed.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 126:
> 124: Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1");
> 125: FileUtils.deleteFileTreeWithRetry(pkg1Dir);
> 126: }
I'm not convinced the `@AfterClass` method is necessary. Pre-cleanup is
already done on LL94-95. And occasionally, test-generated artifacts are
helpful in post-mortem analysis.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 158:
> 156: private void match(final String actual, final String expected) {
> 157: System.out.println("actual:" + actual + "expected:" + expected);
> 158: assert ((actual == null && expected == null) ||
> actual.trim().equals(expected.trim()));
Out of curiousity, why is the trim() now needed ?
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 180:
> 178: info(c, encClass, annotation.desc());
> 179: check(c, encClass, "" + encClass, annotation.encl(),
> c.getSimpleName(), annotation.simple(),
> 180: c.getCanonicalName(), annotation.hasCanonical() ?
> annotation.canonical() : null);
This looks like an unneeded formatting change
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 187:
> 185: check(array, array.getEnclosingClass(), "", "",
> array.getSimpleName(),
> 186: annotation.simple() + "[]", array.getCanonicalName(),
> annotation.hasCanonical()
> 187: ? annotation.canonical() + "[]" : null);
This looks like an unneeded formatting change
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 197:
> 195: testClass((Class) f.get(tests), annotation, f);
> 196: } catch (AssertionError ex) {
> 197: System.err.println("Error in " +
> tests.getClass().getName() + "." + f.getName());
This looks like an unneeded formatting change
-
Changes requested by bchristi (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Fri, 22 Jan 2021 16:57:41 GMT, Mahendra Chhipa
wrote:
>> Can this be done all in `EnclosingClassTest.java`, without a new
>> `RunEnclosingClassTest.java`?
>>
>> Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
>> just need to
>> change the `test()` calls to use reflection - something along the lines of:
>>
>>> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
>
> Review comments implemented.
Can the new code be added to the existing `NonJavaNames.java` instead of adding
a new `NonJavaNameTest.java` file?
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Wed, 20 Jan 2021 17:27:43 GMT, Mahendra Chhipa
wrote:
> https://bugs.openjdk.java.net/browse/JDK-8183372
Can this be done all in `EnclosingClassTest.java`, without a new
`RunEnclosingClassTest.java`?
Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
just need to
change the `test()` calls to use reflection - something along the lines of:
> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: [jdk16] RFR: 8259298: broken link in Stream::toList spec
On Mon, 11 Jan 2021 23:15:07 GMT, Stuart Marks wrote: > Just fixing a broken link. Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/106
[jdk16] Integrated: 8258007: Add instrumentation to NativeLibraryTest
On Fri, 18 Dec 2020 22:33:11 GMT, Brent Christian wrote: > This change adds some extra test output for NativeLibraryTest, primarily via > an update to the ForceGC utility class. > > It was observed that there was nothing preventing the Cleaner from cleaning > the short-lived Object that ForceGC registers before > await()/doit()/System.gc() is even called. > > The new 'o' reference is kept alive until FoceGC.await() has been called. > > We should find out a little more the next time NativeLibraryTest fails (or > perhaps it won't fail anymore!) This pull request has now been integrated. Changeset: e680ebeb Author:Brent Christian URL: https://git.openjdk.java.net/jdk16/commit/e680ebeb Stats: 13 lines in 2 files changed: 9 ins; 0 del; 4 mod 8258007: Add instrumentation to NativeLibraryTest Reviewed-by: mchung, naoto - PR: https://git.openjdk.java.net/jdk16/pull/53
Re: [jdk16] RFR: 8258007: Add instrumentation to NativeLibraryTest [v3]
On Fri, 18 Dec 2020 23:31:35 GMT, Naoto Sato wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> format() -> println()
>
> test/lib/jdk/test/lib/util/ForceGC.java line 48:
>
>> 46: for (int i = 0; i < 10; i++) {
>> 47: System.gc();
>> 48: System.out.format("doit %d: gc %d%n", iter, i);
>
> Could be better to keep using println(), if performance is important.
I agree we don't need format() here
-
PR: https://git.openjdk.java.net/jdk16/pull/53
Re: [jdk16] RFR: 8258007: Add instrumentation to NativeLibraryTest [v3]
> This change adds some extra test output for NativeLibraryTest, primarily via > an update to the ForceGC utility class. > > It was observed that there was nothing preventing the Cleaner from cleaning > the short-lived Object that ForceGC registers before > await()/doit()/System.gc() is even called. > > The new 'o' reference is kept alive until FoceGC.await() has been called. > > We should find out a little more the next time NativeLibraryTest fails (or > perhaps it won't fail anymore!) Brent Christian has updated the pull request incrementally with one additional commit since the last revision: format() -> println() - Changes: - all: https://git.openjdk.java.net/jdk16/pull/53/files - new: https://git.openjdk.java.net/jdk16/pull/53/files/54a9e6d1..f0dd43ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk16=53=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk16=53=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk16/pull/53.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/53/head:pull/53 PR: https://git.openjdk.java.net/jdk16/pull/53
Re: [jdk16] RFR: 8258007: Add instrumentation to NativeLibraryTest [v2]
On Fri, 18 Dec 2020 22:43:16 GMT, Mandy Chung wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add sleep to ForceGC.await()
>
> test/lib/jdk/test/lib/util/ForceGC.java line 49:
>
>> 47: System.gc();
>> 48: System.out.format("doit %d: gc %d%n", iter, i);
>> 49: if (cleanerInvoked.await(1L, TimeUnit.SECONDS)) {
>
> If the object that the test waits for garbage collected takes more GC cycles
> than the object registered in ForceGC's cleaner, this will return
> immediately. If you are concerned, maybe if the count down latch already
> becomes zero, it should call System.sleep before the next System.gc().
I am a little concerned. But I think it makes more sense to sleep in await(),
between calls of doit(). I'll push an update.
-
PR: https://git.openjdk.java.net/jdk16/pull/53
Re: [jdk16] RFR: 8258007: Add instrumentation to NativeLibraryTest [v2]
> This change adds some extra test output for NativeLibraryTest, primarily via > an update to the ForceGC utility class. > > It was observed that there was nothing preventing the Cleaner from cleaning > the short-lived Object that ForceGC registers before > await()/doit()/System.gc() is even called. > > The new 'o' reference is kept alive until FoceGC.await() has been called. > > We should find out a little more the next time NativeLibraryTest fails (or > perhaps it won't fail anymore!) Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Add sleep to ForceGC.await() - Changes: - all: https://git.openjdk.java.net/jdk16/pull/53/files - new: https://git.openjdk.java.net/jdk16/pull/53/files/9b5e57ca..54a9e6d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk16=53=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk16=53=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk16/pull/53.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/53/head:pull/53 PR: https://git.openjdk.java.net/jdk16/pull/53
[jdk16] RFR: 8258007: Add instrumentation to NativeLibraryTest
This change adds some extra test output for NativeLibraryTest, primarily via an update to the ForceGC utility class. It was observed that there was nothing preventing the Cleaner from cleaning the short-lived Object that ForceGC registers before await()/doit()/System.gc() is even called. The new 'o' reference is kept alive until FoceGC.await() has been called. We should find out a little more the next time NativeLibraryTest fails (or perhaps it won't fail anymore!) - Commit messages: - Don't keep ForceGC'ing until we see the expected count - Add comment about new Object reference in ForceGC - fix spacing - Keep ForceGC'ing until we see the expected count - Add strongly-referenced Object to prevent cleaning before await() is called Changes: https://git.openjdk.java.net/jdk16/pull/53/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk16=53=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258007 Stats: 10 lines in 2 files changed: 6 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk16/pull/53.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/53/head:pull/53 PR: https://git.openjdk.java.net/jdk16/pull/53
Integrated: 8253497: Core Libs Terminology Refresh
On Mon, 14 Dec 2020 19:36:48 GMT, Brent Christian wrote: > This is part of an effort in the JDK to replace archaic/non-inclusive words > with more neutral terms (see JDK-8253315 for details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > 1. grandfathered -> legacy > 2. blacklist -> filter or reject > 3. whitelist -> allow or accept > 4. master -> coordinator > 5. slave -> worker > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. This pull request has now been integrated. Changeset: b2f03554 Author:Brent Christian URL: https://git.openjdk.java.net/jdk/commit/b2f03554 Stats: 82 lines in 15 files changed: 1 ins; 0 del; 81 mod 8253497: Core Libs Terminology Refresh Reviewed-by: naoto, kcr, rriggs, joehw, bpb, smarks, alanb - PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v5]
> This is part of an effort in the JDK to replace archaic/non-inclusive words > with more neutral terms (see JDK-8253315 for details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > 1. grandfathered -> legacy > 2. blacklist -> filter or reject > 3. whitelist -> allow or accept > 4. master -> coordinator > 5. slave -> worker > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Use quotes instead of @code in Locale - Changes: - all: https://git.openjdk.java.net/jdk/pull/1771/files - new: https://git.openjdk.java.net/jdk/pull/1771/files/ba586413..03264330 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1771=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1771=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1771/head:pull/1771 PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v4]
On Wed, 16 Dec 2020 07:10:41 GMT, Alan Bateman wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> improve SERIAL_FILTER_PATTERN comment
>
> src/java.base/share/classes/java/util/Locale.java line 1649:
>
>> 1647: * This implements the 'Language-Tag' production of BCP47, and
>> 1648: * so supports legacy (regular and irregular, referred to as
>> 1649: * {@code Type: grandfathered} in BCP47) as well as
>
> You might consider putting quotes around "Type; grandfathered" here,
> otherwise looks good.
Yes, having that in quotes instead of in {@code} would be consistent with the
rest of Locale.java. I will change that.
-
PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v4]
> This is part of an effort in the JDK to replace archaic/non-inclusive words > with more neutral terms (see JDK-8253315 for details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > 1. grandfathered -> legacy > 2. blacklist -> filter or reject > 3. whitelist -> allow or accept > 4. master -> coordinator > 5. slave -> worker > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: improve SERIAL_FILTER_PATTERN comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/1771/files - new: https://git.openjdk.java.net/jdk/pull/1771/files/b8cd8b6d..ba586413 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1771=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1771=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1771/head:pull/1771 PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v3]
On Tue, 15 Dec 2020 22:13:58 GMT, Stuart Marks wrote: >> It's an adverb, since it's the act of 'defining' that is being done too >> restrictively or broadly. If you want to have an adjective you would need to >> rephrase it so it applies to the noun, like 'defining a too restrictive >> accept-list'. My non-native English vibes would still prefer the former. > > I'd suggest `... as defining an allow-list that is too narrow or a > reject-list that is too wide may prevent ...`. > > (edited to remove hyphen from "too-wide") Even better! - PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v3]
> This is part of an effort in the JDK to replace archaic/non-inclusive words > with more neutral terms (see JDK-8253315 for details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > 1. grandfathered -> legacy > 2. blacklist -> filter or reject > 3. whitelist -> allow or accept > 4. master -> coordinator > 5. slave -> worker > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. Brent Christian has updated the pull request incrementally with three additional commits since the last revision: - This is the controller - use 'controller' in Assert.java - use 'peer' in CloseRegisteredChannel.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1771/files - new: https://git.openjdk.java.net/jdk/pull/1771/files/29525f05..b8cd8b6d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1771=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1771=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1771/head:pull/1771 PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v2]
On Tue, 15 Dec 2020 22:02:00 GMT, Lance Andersen wrote:
>> test/jdk/java/lang/ClassLoader/Assert.java line 65:
>>
>>> 63:
>>> 64: int switchSource = 0;
>>> 65: if (args.length == 0) { // This is the coordinator version
>>
>> Perhaps s/coordinator/controller/?
>
> Let's change it to:
>
> // This is the controller
Will do.
-
PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v2]
On Tue, 15 Dec 2020 07:32:12 GMT, Alan Bateman wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> updates, per code review > > test/jdk/java/nio/channels/SocketChannel/CloseRegisteredChannel.java line 45: > >> 43: SocketChannel client = SocketChannel.open (); >> 44: client.connect (new InetSocketAddress >> (InetAddress.getLoopbackAddress(), port)); >> 45: SocketChannel channel = server.accept (); > > Can you rename this to "peer" instead? (we can remove the spurious space > after accept while we are there). Yes, I will name it, "peer", and remove the extra space on the affected lines. - PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v2]
> This is part of an effort in the JDK to replace archaic/non-inclusive words > with more neutral terms (see JDK-8253315 for details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > 1. grandfathered -> legacy > 2. blacklist -> filter or reject > 3. whitelist -> allow or accept > 4. master -> coordinator > 5. slave -> worker > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: updates, per code review - Changes: - all: https://git.openjdk.java.net/jdk/pull/1771/files - new: https://git.openjdk.java.net/jdk/pull/1771/files/4efa5d43..29525f05 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1771=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1771=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1771/head:pull/1771 PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh
On Mon, 14 Dec 2020 21:08:35 GMT, Joe Wang wrote: >> This is part of an effort in the JDK to replace archaic/non-inclusive words >> with more neutral terms (see JDK-8253315 for details). >> >> Here are the changes covering core libraries code and tests. Terms were >> changed as follows: >> 1. grandfathered -> legacy >> 2. blacklist -> filter or reject >> 3. whitelist -> allow or accept >> 4. master -> coordinator >> 5. slave -> worker >> >> Addressing similar issues in upstream 3rd party code is out of scope of this >> PR. Such changes will be picked up from their upstream sources. > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectorServer.java > line 152: > >> 150: * >> 151: * Care must be taken when defining such a filter, as defining >> 152: * an accept-list too restrictive or a too-wide reject-list may > > would "an allow-list too restrictive or a reject-list too wide" read better? I agree that there is room for improvement here. How about: "...an allow-list too restrictively, or a reject-list too broadly, may..." ? - PR: https://git.openjdk.java.net/jdk/pull/1771
RFR: 8253497: Core Libs Terminology Refresh
This is part of an effort in the JDK to replace archaic/non-inclusive words with more neutral terms (see JDK-8253315 for details). Here are the changes covering core libraries code and tests. Terms were changed as follows: 1. grandfathered -> legacy 2. blacklist -> filter or reject 3. whitelist -> allow or accept 4. master -> coordinator 5. slave -> worker Addressing similar issues in upstream 3rd party code is out of scope of this PR. Such changes will be picked up from their upstream sources. - Commit messages: - Terminology Cleanup - corelibs terminology refresh - bchristi Changes: https://git.openjdk.java.net/jdk/pull/1771/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1771=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253497 Stats: 82 lines in 15 files changed: 1 ins; 0 del; 81 mod Patch: https://git.openjdk.java.net/jdk/pull/1771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1771/head:pull/1771 PR: https://git.openjdk.java.net/jdk/pull/1771
