On Tue, 14 Apr 2026 09:29:49 GMT, Marcono1234 <[email protected]> wrote:
>> 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. Indeed, my intended scope for this PR is only to replace finalizer functionality with Cleaner. Therefore, I'd like to leave the rest of the code as is. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3211630638
