[crac] RFR: Ensure all notifications finish even if only daemon threads remain [v2]

[Anton Kozlov]

On Thu, 4 May 2023 15:04:11 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Test update
>
> src/java.base/share/classes/jdk/crac/Core.java line 254:
> 
>> 252:                 // The notifications are done on the original thread.
>> 253:                 CountDownLatch start = new CountDownLatch(1);
>> 254:                 CountDownLatch finish = new CountDownLatch(1);
> 
> Rather than 2 CountDownLatches you could use either single `CyclicBarrier` (awaiting twice), or even better a `Phaser` that has API for non-interruptible wait.

Since both of suggested options are reusable, having separted events is cleaner and reduce chances to accidentally await when that was intended.

> test/jdk/jdk/crac/DaemonAfterRestore.java line 82:
> 
>> 80:             System.out.println("worker thread finish");
>> 81:         });
>> 82:         workerThread.setDaemon(false);
> 
> Unnecessary, thread created from main thread (non-daemon) is not a daemon either.

Otherwise there should be an assert, so it's more straightforward to set the mode.

> test/jdk/jdk/crac/DaemonAfterRestore.java line 98:
> 
>> 96:             @Override
>> 97:             public void afterRestore(Context<? extends Resource> context) throws Exception {
>> 98:                 Thread.sleep(3000);
> 
> Could we avoid extending the testsuite run by 3 seconds? I know we're trying to assert that 'nothing happens' rather then replacing waiting for an event by a timed wait. If we can't check that the destroy VM thread had a chance to work, I suggest at least reducing this (say 100ms?).

100ms does not look as a good candidate (comparable to a single scheduling period). So 3 sec does not look to bad to avoid interrmittent false positive because sleep() finished before VM has terminated.

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

PR Review Comment: https://git.openjdk.org/crac/pull/62#discussion_r1185290868
PR Review Comment: https://git.openjdk.org/crac/pull/62#discussion_r1185291965
PR Review Comment: https://git.openjdk.org/crac/pull/62#discussion_r1185295106