[crac] RFR: Prevent concurrent cleanup by cleaner thread and checkpoint notifications

[Radim Vansa]

On Wed, 24 May 2023 13:26:47 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> We block the cleaner thread to prevent race conditions between this thread and checkpointing thread invoking clean().
>> When the cleanup starts in cleaner thread the checkpoint will skip it, but without waiting for the cleanup to finish (which might be critical for the checkpoint, e.g. closing FDs).
>> The limitation is that code performing C/R must not wait on any task completed by the cleaner.
>
> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 151:
> 
>> 149:                         while (blockForCheckpoint) {
>> 150:                             wait();
>> 151:                         }
> 
> Once we've got here, is it possible to ensure Cleaners has been called, and drop separate registration of Cleaners?
> 
> A concurrent cleaner registration is not a problem, as that depends on GC which is not predictable. I.e. if that happens, a slight race may also cause the cleaner to run after restore.

I don't follow. PhantomCleanableRefs are cleaned strictly after this point (these have later priority), other cleaners will be queued up by GC but not called until restore.

> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 180:
> 
>> 178:         // critical for the checkpoint, e.g. closing FDs).
>> 179:         // The limitation is that code performing C/R must not wait on any task
>> 180:         // completed by the cleaner.
> 
> Could you elaborate why there is this limitation?

The thread is blocked until `afterRestore`. PhantomCleanableRefs are cleaned independently within its own priority class (after the thread is stopped), but any other cleanup tasks simply won't happen. Therefore if C/R after this point waits for the cleanup, it will deadlock.

-------------

PR Review Comment: https://git.openjdk.org/crac/pull/73#discussion_r1205103396
PR Review Comment: https://git.openjdk.org/crac/pull/73#discussion_r1205100248