On Wed, 20 Apr 2022 19:59:16 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>>> Won't there be a small performance hit (perhaps negligible) for code that >>> already calls clearPassword? >> >> The impact should be minimal. If clearPassword() has been called, the >> cleanup (Cleanerable.clean()) won't happen again in the finalization or >> setPassword cleanup. >> >>> A specification clarification would provide clarity to applications that >>> they do not have to call clearPassword in between calls to setPassword. >> >> As far as I know from the JDK code, it might be not common to call >> clearPassword in between calls to setPassword. I would like to have >> applications calling clearPassword() methods as before, if it is not missed, >> to speed up the collection rather than rely on finalization. >> >> The relationship among setPassword, getPassword and clearPassword() is >> complicated. I fully agree that the spec should be clarified. I would like >> to have the clarify update in another PR, and have this one focus on cleanup >> if an application forget to call clearPassword properly. > >> > Won't there be a small performance hit (perhaps negligible) for code that >> > already calls clearPassword? >> >> The impact should be minimal. If clearPassword() has been called, the >> cleanup (Cleanerable.clean()) won't happen again in the finalization or >> setPassword cleanup. > > I don't think that is true, but maybe I am missing something. From looking at > the code, it appears a `clearPassword` followed by a `setPassword` would call > `Arrays.fill` twice on the same password. I think this can be fixed by > setting the cleaner to null in the `clearPassword` method after the password > has been cleared. I think this would address my concern and we may not need a > spec. update (though I want to think it thru one more time). > >> > A specification clarification would provide clarity to applications that >> > they do not have to call clearPassword in between calls to setPassword. >> >> As far as I know from the JDK code, it might be not common to call >> clearPassword in between calls to setPassword. I would like to have >> applications calling clearPassword() methods as before, if it is not missed, >> to speed up the collection rather than rely on finalization. > > Yes, I agree calling `clearPassword` should be recommended as a best practice > and callers should not solely rely on cleaners. > >> The relationship among setPassword, getPassword and clearPassword() is >> complicated. I fully agree that the spec should be clarified. I would like >> to have the clarify update in another PR, and have this one focus on cleanup >> if an application forget to call clearPassword properly. > > See above. Calling `Cleanable.clean()` calls the `Runnable` action at-most-once. Each `Cleanable` inserted in a list when it is created and is removed when `clear()` or `clean()` is invoked. The action is called only if it is still in the list. So there is no extra overhead. There is no harm in clearing the cleanable field; but it does not save much. The code in `clearPassword` can be simplified and only test `cleanable != null`; it will be null unless there is an inputPassword to clean. ------------- PR: https://git.openjdk.java.net/jdk/pull/8272
