[crac] RFR: 8357984: [CRaC] Improve new properties installation code

[Radim Vansa]

On Wed, 28 May 2025 15:46:42 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

> Rewrites the code which installs new properties on restore to make it more concise by using direct array iteration and lambdas.
> 
> Note that initially the lambdas were removed from this code in #75 because they triggered a resource registration in a blocking context during C/R which led to a deadlock. But as I understand #73 made it possible to use lambdas during C/R.
> 
> I also noted that there are no tests for new properties so I added one.

While I also think that it should be possible to use lambdas, the deadlock was happening as a race condition, and I wonder if we could gain more confidence about this (stress test?). Or, do we consider all the restores happening in a single testsuite run sufficient?

src/java.base/share/classes/jdk/internal/crac/mirror/Core.java line 198:

> 196:             for (var prop : newProperties) {
> 197:                 String[] pair = prop.split("=", 2);
> 198:                 AccessController.doPrivileged((PrivilegedAction<Void>) () -> {

With SecurityManager obsoleted, we don't have to use `AccessController.doPrivileged` at all, do we?

src/java.base/share/classes/jdk/internal/crac/mirror/Core.java line 199:

> 197:                 String[] pair = prop.split("=", 2);
> 198:                 AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
> 199:                     System.setProperty(pair[0], pair[1]);

Looks like you have omitted the case when `prop` does not contain `=`, we would OOTB here. I guess the code that creates the properties would always add `=` (+ empty string), but let's be on the safe side.

test/jdk/jdk/crac/NewPropertyTest.java line 41:

> 39:         final var builder = new CracBuilder();
> 40:         builder.doCheckpoint();
> 41:         builder.javaOption("k", "v");

Please add `builder.vmOption("-Dfoo")` and maybe `builder.javaOptions("bar", "")` to assert on the empty property case.
Also worth checking that existing property is overridden, not only that new property is defined.

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

PR Review: https://git.openjdk.org/crac/pull/233#pullrequestreview-2877246511
PR Review Comment: https://git.openjdk.org/crac/pull/233#discussion_r2113306628
PR Review Comment: https://git.openjdk.org/crac/pull/233#discussion_r2113313559
PR Review Comment: https://git.openjdk.org/crac/pull/233#discussion_r2113320823