RFR: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run

[Kim Barrett]

On Mon, 10 Mar 2025 23:14:23 GMT, Brent Christian <bchristi at openjdk.org> wrote:

>> Please review this revision of a previously puzzling comment intending to
>> provide the rationale for a bit of non-obvious code.
>
> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 142:
> 
>> 140:                 // while there are registered cleanables for other objects.
>> 141:                 // If the application cleans all remaining cleanables, there
>> 142:                 // won't be any references enqueued to unblock this.  Using a
> 
> I agree that calling `queue.remove()` with a timeout is the right approach.
> But I have a question:
> In the case where the Cleaner's `CleanerCleanable` has already run, and we get to processing the last registered cleanable on `activeList`:
> When we do the `ref.clean()`, the `activeList` becomes empty, and we'll drop out of the `while()` loop. So I'm not seeing how we would attempt another `queue.remove()` in this case.
> What am I missing?

You are missing that this loop is not the only place that potentially removes
references from `activeList`. The application may also do so, when directly
cleaning (as part of a `close()` operation or the like). So the cleaner's
cleanable may be dropped, removed, and cleaned but with live entries still in
`activeList`, and this loop ends up blocked in `remove` because there's
nothing for it to do.  The application later closes all of the remaining
entries in the `activeList`, which doesn't cause anything to be enqueued on
the cleaner's queue, so the cleaner thread remains blocked in `remove`.  But
now `activeList` is empty and will never be added to, and the queue is also
empty, and the thread is blocked in `remove` with nothing (other than the
timeout) to break it out.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23952#discussion_r1992205243