Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Tue, 12 May 2026 04:30:27 GMT, Brent Christian wrote:
>> test/jdk/sun/security/smartcardio/TestCleaner.java line 71:
>>
>>> 69: Card card = terminal.connect("*");
>>> 70: if (card == null) {
>>> 71: System.out.println("Skipping the test: " +
>>
>> This should be a skipped exception imo, it will look like it's passed
>> otherwise
>
> This test is meant only to test a specific thing - that the new cleaning
> action does not prevent the Card object (CardImpl, to be precise) from being
> collected.
>
> If no Card object can be obtained, this can't be tested. I think it can be
> skipped. It's common for other tests in the smartcardio/ test directory to be
> skipped if conditions aren't right.
>
> In my view, if the terminal senses that a card is present, but connect() does
> not yield a Card object, that indicates a problem in some other part of the
> system. I would expect it to be caught by one of the other tests.
My preference here would be to keep the same testing approach as pre-existing
for this part of the codebase, and if any changes / improvements are desired
that they are tackled as a separate PR and across the board.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3225266912
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Mon, 11 May 2026 11:54:02 GMT, Mikhail Yankelevich
wrote:
>> Brent Christian has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into smartcardio
>> - update example code in javax.smartcardio package doc to use try-finally
>> - minor comment update
>> - add missed clean() calls, update comments
>> - fix imports and copyright years
>> - add test case
>> - convert CardImpl finalizer to use Cleaner
>
> test/jdk/sun/security/smartcardio/TestCleaner.java line 71:
>
>> 69: Card card = terminal.connect("*");
>> 70: if (card == null) {
>> 71: System.out.println("Skipping the test: " +
>
> This should be a skipped exception imo, it will look like it's passed
> otherwise
This test is meant only to test a specific thing - that the new cleaning action
does not prevent the Card object (CardImpl, to be precise) from being collected.
If no Card object can be obtained, this can't be tested. I think it can be
skipped. It's common for other tests in the smartcardio/ test directory to be
skipped if conditions aren't right.
In my view, if the terminal senses that a card is present, but connect() does
not yield a Card object, that indicates a problem in some other part of the
system. I would expect it to be caught by one of the other tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3223710938
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Thu, 16 Apr 2026 22:28:53 GMT, Brent Christian 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).
>
> Brent Christian has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Merge branch 'master' into smartcardio
> - update example code in javax.smartcardio package doc to use try-finally
> - minor comment update
> - add missed clean() calls, update comments
> - fix imports and copyright years
> - add test case
> - convert CardImpl finalizer to use Cleaner
test/jdk/sun/security/smartcardio/TestCleaner.java line 71:
> 69: Card card = terminal.connect("*");
> 70: if (card == null) {
> 71: System.out.println("Skipping the test: " +
This should be a skipped exception imo, it will look like it's passed otherwise
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3218650654
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Fri, 8 May 2026 23:13:54 GMT, Brent Christian wrote:
>> test/jdk/sun/security/smartcardio/TestCleaner.java line 54:
>>
>>> 52: * CardImpl from becoming unreachable and being collected/cleaned.
>>> 53: */
>>> 54: public class TestCleaner extends Utils {
>>
>> Since this is a manual test have you considered using a ui for instructions?
>> I think the go-to method now is `UIBuilder`. e.g. :
>>
>> final JDialog dialog = new UIBuilder.DialogBuilder()
>> .setTitle("Title")
>> .setInstruction(instruction)
>> .setMessage(message)
>> .setPassAction(e -> pass())
>> .setFailAction(e -> fail())
>> .setCloseAction(this::abort)
>> .build();
>>
>> https://github.com/openjdk/jdk/blob/master/test/jdk/com/sun/security/auth/callback/TextCallbackHandler/Password.java#L85
>
> Hi, Mikhail
>
> I've made `TestCleaner.java` similar to the other `@run main/manual` tests in
> `test/jdk/sun/security/smartcardio/`, which don't create a GUI explicitly. I
> admit that I don't know the specifics of how these tests are run, but AFAIK
> one can still bring up the gui using `jtreg -gui ...`.
>
> As with the other tests, `TestCleaner` "requires special hardware" (a real or
> simulated JavaCard).
> As long as a `CardTerminal.connect()` can be performed, `TestCleaner` runs on
> its own, and doesn't require manual intervention (or, IMO, instructions).
My worry was the request to insert the card if it is not inserted. Could it be
a fail case instead? Then the test won't need any user interaction at all.
Alternatively, a popup would work better than the message since it is very
likely to be missed from my experience
What do you think ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3218640978
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Thu, 16 Apr 2026 22:28:53 GMT, Brent Christian 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).
>
> Brent Christian has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Merge branch 'master' into smartcardio
> - update example code in javax.smartcardio package doc to use try-finally
> - minor comment update
> - add missed clean() calls, update comments
> - fix imports and copyright years
> - add test case
> - convert CardImpl finalizer to use Cleaner
src/java.smartcardio/share/classes/javax/smartcardio/package-info.java line 2:
> 1: /*
> 2: * Copyright (c) 2005, 2020, Oracle and/or its affiliates. All rights
> reserved.
Update copyright?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3214741220
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Wed, 15 Apr 2026 09:11:12 GMT, Mikhail Yankelevich
wrote:
>> Brent Christian has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into smartcardio
>> - update example code in javax.smartcardio package doc to use try-finally
>> - minor comment update
>> - add missed clean() calls, update comments
>> - fix imports and copyright years
>> - add test case
>> - convert CardImpl finalizer to use Cleaner
>
> test/jdk/sun/security/smartcardio/TestCleaner.java line 54:
>
>> 52: * CardImpl from becoming unreachable and being collected/cleaned.
>> 53: */
>> 54: public class TestCleaner extends Utils {
>
> Since this is a manual test have you considered using a ui for instructions?
> I think the go-to method now is `UIBuilder`. e.g. :
>
> final JDialog dialog = new UIBuilder.DialogBuilder()
> .setTitle("Title")
> .setInstruction(instruction)
> .setMessage(message)
> .setPassAction(e -> pass())
> .setFailAction(e -> fail())
> .setCloseAction(this::abort)
> .build();
>
> https://github.com/openjdk/jdk/blob/master/test/jdk/com/sun/security/auth/callback/TextCallbackHandler/Password.java#L85
Hi, Mikhail
I've made `TestCleaner.java` similar to the other `@run main/manual` tests in
`test/jdk/sun/security/smartcardio/`, which don't create a GUI explicitly. I
admit that I don't know the specifics of how these tests are run, but AFAIK one
can still bring up the gui using `jtreg -gui ...`.
As with the other tests, `TestCleaner` "requires special hardware" (a real or
simulated JavaCard).
As long as a `CardTerminal.connect()` can be performed, `TestCleaner` runs on
its own, and doesn't require manual intervention (or, IMO, instructions).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3211825113
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Tue, 14 Apr 2026 09:29:49 GMT, Marcono1234 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
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
On Tue, 14 Apr 2026 09:32:23 GMT, Marcono1234 wrote:
>> Brent Christian has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into smartcardio
>> - update example code in javax.smartcardio package doc to use try-finally
>> - minor comment update
>> - add missed clean() calls, update comments
>> - fix imports and copyright years
>> - add test case
>> - convert CardImpl finalizer to use Cleaner
>
> src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java
> line 170:
>
>> 168: try {
>> 169: if (e.code == SCARD_W_REMOVED_CARD) {
>> 170: context.state = State.REMOVED;
>
> Related to https://github.com/openjdk/jdk/pull/30683#discussion_r3071989746,
> should instead this here actually de-register the Cleaner?
Yes, done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30683#discussion_r3211548106
Re: RFR: 8380391: Update java.smartcardio finalizer to use Cleaner [v6]
> 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).
Brent Christian has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains seven additional
commits since the last revision:
- Merge branch 'master' into smartcardio
- update example code in javax.smartcardio package doc to use try-finally
- minor comment update
- add missed clean() calls, update comments
- fix imports and copyright years
- add test case
- convert CardImpl finalizer to use Cleaner
-
Changes:
- all: https://git.openjdk.org/jdk/pull/30683/files
- new: https://git.openjdk.org/jdk/pull/30683/files/95937ea6..fce7e3f9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=30683&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=30683&range=04-05
Stats: 88910 lines in 2204 files changed: 47708 ins; 25904 del; 15298 mod
Patch: https://git.openjdk.org/jdk/pull/30683.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/30683/head:pull/30683
PR: https://git.openjdk.org/jdk/pull/30683
