On Sun, 12 Apr 2026 14:58:49 GMT, Marcono1234 <[email protected]> wrote:
>> This is a pull request to convert the finalizer in >> `sun.security.smartcardio.CardImpl` to use Cleaner instead. The relevant >> state is refactored into a Context object, in the standard fashion. >> >> This change uses the recommended `try`/`finally`/`reachabilityFence()` >> technique to prevent races between the program thread and the Cleaner >> thread, per the `Reference.reachabilityFence()` API Note: >> >>> _there is a race between the program thread running the method, and the >>> cleanup thread running the Cleaner or finalizer. The cleanup thread could >>> free a resource, followed by the program thread (still running the method) >>> attempting to access the now-already-freed resource. Use of >>> reachabilityFence can prevent this race by ensuring that the object remains >>> strongly reachable._ >> >> See >> [Reference.reachabilityFence()](https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/lang/ref/Reference.html#reachabilityFence(java.lang.Object)) >> and [java.lang.ref - Memory >> Visibility](https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/lang/ref/package-summary.html#memory-consistency-properties-heading) >> for details / background info. >> >> The test creates a `Card` object, and allows it to be collected. This >> confirms that the new cleaning action does not hold onto the Owner ("this") >> object, per the >> [Cleaner](https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/lang/ref/Cleaner.html) >> class API Note ("_it is important that the object implementing the cleaning >> action does not hold references to the object_"). >> >> I've tried to make the test similar to nearby JavaCard tests. I tried it on >> Windows using the latest JavaCard Simulator. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java > line 151: > >> 149: context.state = State.REMOVED; >> 150: // Cleaner no longer needs to track >> 151: cleanable.clean(); // FIXME: why? > > Is this FIXME still open? > > Though it seems to be the same behavior as before; there it also set `state = > State.REMOVED`, making `finalize()` effectively nop. > So is this mostly a "why was this done like this before"? The FIXME is no longer open. :) I'll remove it. I've had this patch sitting around locally for..."a while". I think what happened was that I added the `cleanable.clean()` line early on, and the FIXME was more recent, to remind me to figure out why I added it. `cleanable.clean()` here is not so much to run the (noop) cleanup code, but to promptly remove itself from the Cleaner infrastructure. This lightens the load on the GC - there's one less item to track in terms of reference processing, etc. Finalizers don't have such functionality. AFAICT, `isValid()` is only called from [TerminalImpl.connect()](https://github.com/openjdk/jdk/blob/master/src/java.smartcardio/share/classes/sun/security/smartcardio/TerminalImpl.java#L66). The next line in the code path of returning false from `card.isValid()` is setting `card = null`. So `card` is done being used. For the record, I noticed that `TerminalImpl.connect()` is a synchronized method - `this` will be locked at the time that `card.isValid()` is called, and still locked when `cleanable.clean()` is called, and when the (noop) cleanup code called. It may not have happened this way with the finalizer version. However, given that the locked object isa `TerminalImpl`, I don't believe this is cause for concern. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3076339125
