Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Sat, 9 Apr 2022 15:00:46 GMT, Weijun Wang wrote:
>>> You might try it on other objects:
>>
>> Nice idea to test object collection. I added two test cases.
>>
>>> The `NativeGSSContext` code still needs to be fixed.
>>
>> Could you have more details? I did not catch the comment about
>> NativeGSSContext.
>
>> Could you have more details? I did not catch the comment about
>> NativeGSSContext.
>
> When `NativeGSSContext(GSSNameElement peer, GSSCredElement myCred, int time,
> GSSLibStub stub)` is called, `pContext` is 0 and you haven't registered the
> cleaner. Later, when `initSecContext()` is called, it calls into
> `cStub.initContext()` and this native method will set `pContext` if a context
> is established. Since you haven't registered the cleaner in the ctor, this
> native context will not be released at the end.
>
> So one solution is to add a `setNativeContext(long pContext)` method and when
> this method is called you register the cleaner. Now, inside the native
> method, instead of setting the `pContext` field directly you can call this
> setter method. Or, move `cStub` and `pContext` into a new static inner class
> and let `disposerFor` work on it.
@wangweij In the following constructor, I'm not sure if the assert right.
NativeGSSContext(long pCtxt, GSSLibStub stub) throws GSSException {
assert(pContext != 0);
...
Should it be `assert(pCtxt != 0);`?
-
PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Sat, 9 Apr 2022 06:22:51 GMT, Xue-Lei Andrew Fan wrote: > Could you have more details? I did not catch the comment about > NativeGSSContext. When `NativeGSSContext(GSSNameElement peer, GSSCredElement myCred, int time, GSSLibStub stub)` is called, `pContext` is 0 and you haven't registered the cleaner. Later, when `initSecContext()` is called, it calls into `cStub.initContext()` and this native method will set `pContext` if a context is established. Since you haven't registered the cleaner in the ctor, this native context will not be released at the end. So one solution is to add a `setNativeContext(long pContext)` method and when this method is called you register the cleaner. Now, inside the native method, instead of setting the `pContext` field directly you can call this setter method. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Fri, 8 Apr 2022 13:50:25 GMT, Weijun Wang wrote: > You might try it on other objects: Nice idea to test object collection. I added two test cases. > The `NativeGSSContext` code still needs to be fixed. Could you have more details? I did not catch the comment about NativeGSSContext. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Fri, 8 Apr 2022 13:50:25 GMT, Weijun Wang wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update copyright year
>
> @bchristi-git showed me this test. You might try it on other objects:
>
> import org.ietf.jgss.GSSManager;
> import org.ietf.jgss.GSSName;
>
> import java.util.WeakHashMap;
>
> public class A1 {
> private static WeakHashMap whm = new WeakHashMap();
> public static void main(String[] args) throws Exception {
> System.setProperty("sun.security.nativegss.debug", "true");
> System.setProperty("sun.security.jgss.native", "true");
> GSSName e = GSSManager.getInstance().createName("u1",
> GSSName.NT_USER_NAME);
> whm.put(e, null);
> e = null;
> for (int i = 0; i < 10; i++) {
> System.gc();
> Thread.sleep(100);
> }
> if (whm.size() > 0) {
> throw new RuntimeException("GSSName still reachable");
> }
> }
> }
>
> The test ensures the objects are GCed and there's no memory leak. You need to
> inspect the debug output to make sure native resources are released. The
> `NativeGSSContext` code still needs to be fixed.
Thanks, @wangweij . Wherever practical, it would be good to include tests to
confirm correct conversions from finalizer to Cleaner -- bugs can be subtle,
and hard to spot.
-
PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 19:21:31 GMT, Xue-Lei Andrew Fan wrote:
>> Please review the update to remove finalizer method in the
>> java.security.jgss module. It is one of the efforts to clean up the use of
>> finalizer method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
@bchristi-git showed me this test. You might try it on other objects:
import org.ietf.jgss.GSSManager;
import org.ietf.jgss.GSSName;
import java.util.WeakHashMap;
public class A1 {
private static WeakHashMap whm = new WeakHashMap();
public static void main(String[] args) throws Exception {
System.setProperty("sun.security.nativegss.debug", "true");
System.setProperty("sun.security.jgss.native", "true");
GSSName e = GSSManager.getInstance().createName("u1",
GSSName.NT_USER_NAME);
whm.put(e, null);
e = null;
for (int i = 0; i < 10; i++) {
System.gc();
Thread.sleep(100);
}
if (whm.size() > 0) {
throw new RuntimeException("GSSElementName still reachable");
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 19:21:31 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year At least for native JGSS, all those native functions that release objects can print out debug messages if `-Dsun.security.nativegss.debug=true`. You can see whether they are called without adding extra `println` calls. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 19:21:31 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year I see a problem. In `NativeGSSContext`, you only assign `cleanable` for one special ctor. In fact, the other 2 ctors (one for initiator and one for acceptor) are more useful. `pContext` is initialized to 0 in these 2 ctors and it's only assigned inside the native code after the context is established. There is no chance to dispose them anymore. I'm not sure what the best way to do this, but maybe you can put `stub` and `pContext` is a separate object and always try to clean it even if `pContext` is not assigned at the beginning. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 20:00:00 GMT, Weijun Wang wrote: > I'm not sure if it's possible to write a test on the cleanup, but at least we > can temporarily add a `println` line there and see if it ever gets called. I did not find a way to test cleanup yet. Yes, a temporary 'println" is what I can use for now. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 19:21:31 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 17:54:35 GMT, Xue-Lei Andrew Fan wrote: >> Hmm, the earlier JCE change would also needs to be updated as it calls a >> cleanup method on the to-be-cleaned object. > >> Hmm, the earlier JCE change would also needs to be updated as it calls a >> cleanup method on the to-be-cleaned object. > > Yes, I will check the cleaner used in the security components and make sure > there is object reference problems. I'm not sure if it's possible to write a test on the cleanup, but at least we can temporarily add a `println` line there and see if it ever gets called. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/22d1bed5..23072ef7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
