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 [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 [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 Hans, thank for the detailed example. I had not fully considered the flow of control in the throwing case. - 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
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 216:
> 214: } finally {
> 215: // Ensure Cleaner does not run until after this method
> completes
> 216: Reference.reachabilityFence(this);
I don't think there is any benefit to the `try{} finally {fence}`.
The reachabilityFence has no executable code. Its only purpose is to keep the
reference in scope alive.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 409:
> 407: } finally {
> 408: // Ensure Cleaner does not run until after this method
> completes
> 409: Reference.reachabilityFence(enumCtx);
Ditto, the try/finally is unnecessary.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 440:
> 438: listArg = ne.listArg;
> 439: } finally {
> 440: // Ensure Cleaner does not run until after this method
> completes
Ditto try/finally is unnecessary.
-
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
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.
-
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
