On Mon, 13 Apr 2026 23:23:26 GMT, Brent Christian <[email protected]> wrote:
>> 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. Thanks for the explanation! > The next line in the code path of returning false from `card.isValid()` is > setting `card = null`. So `card` is done being used. My concern is (and I had assumed your FIXME comment refers to that) that this code sets `State.REMOVED` for _any_ `PCSCException` whereas `handleError(PCSCException)` only sets it if `e.code == SCARD_W_REMOVED_CARD`. So would it be necessary here to have that check too / respectively call `handleError` here instead? And therefore for other `PCSCException`s still call `SCardDisconnect` eventually. Though I am not familiar with this code, and whether this is a problem (potentially it is also out-of-scope for this PR); also I am not an OpenJDK member so feel free to ignore my comment in case you don't think this is an issue. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3078465416
