[crac] RFR: Reseed NativePRNG on checkpoint restore [v3]

[Anton Kozlov]

On Thu, 13 Jan 2022 10:49:46 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:

>> crLock and LOCK_GET_BYTES should be acquired in the particular order to ensure no deadlock can happen. The order is correct now AFAICS, but in the code these locks are used in distant places, checking the locks relation is nontrivial. Someone in the future may add another use of LOCK_GET_BYTES that would go out-of-sync with crLock.
>> 
>> I support using a ReadWriteLock. I'm not sure about how bad the performance was without one (the LOCK_GET_BYTES is there anyway). But some ReadWriteLock is certainly better suited for our task. I'm still not sure the ReentrantReadWriteLock is a perfect one, see another thread.
>
> I do not like to replace LOCK_GET_BYTES with crLock. It will affect performance because of NativePRNG is singleton and the original LOCK_GET_BYTES synchronizes much smaller pieces of code.
> The only standard implementation of ReadWriteLock is ReentrantReadWriteLock. So I think it is OK to use ReentrantReadWriteLock with additional checks I just proposed

OK, I don't insist. Two related locks complicate the code, but crLock intent and the difference from LOCK_GET_BYTES is clearer now. I think the current state is good.

>> Without the debug option, the image may be compromised -- so there should be no option of running without the checks. The currently employed ReentrantReadWriteLock does not block taking Read lock (access to PRNG) while holding Write lock (that is supposed to block access to the PRNG during checkpoint/restore -- but not from the same thread). It would be much more useful to throw an exception "the image is going to be wrong", or at worst to deadlock. But now the implementation does guarantee the security of the image.
>> 
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/concurrent/locks/ReentrantReadWriteLock.html
>
> Thank you. Added the code to verify if NativePRNG is used from the beforeCheckpoint/afterRestore. In such a case, CheckpointException will be thrown.

Thanks, now it looks good. Just as a note, in the similar case I've used IllegalSelectorException [1]. I think IllegalStateException may fit here as well -- we probably need to converge what exception(s) to use. But it is not necessary now.

[1] https://github.com/openjdk/crac/blob/43aba3d502832a5a3d2e9712558f62e0cf93dbbb/src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java#L384

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

PR: https://git.openjdk.java.net/crac/pull/9