[crac] RFR: CRaC related documentation in JDK classes using custom tag [v2]

[Anton Kozlov]

On Tue, 28 Feb 2023 12:33:11 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> As proposed in https://mail.openjdk.org/pipermail/crac-dev/2023-February/000496.html I am adding some CRaC related documentation.
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add timers related docs

src/java.base/share/classes/java/lang/System.java line 551:

> 549:      * of expected bounds, or {@link javax.crac.Context#register(javax.crac.Resource) register}
> 550:      * a resource that will help with adapting after the restore.
> 551:      *

The described behavior looks like a bug. After we got to the concensus in #53, this needs to be updated. Or may be just "TODO" here?

src/java.base/share/classes/java/lang/System.java line 793:

> 791:      * a resource and in the {@link javax.crac.Resource#afterRestore(javax.crac.Context) afterRestore method}
> 792:      * reload system properties, propagating any change.
> 793:      *

The comment above is about standard system properties.

Here we should say that system properties are updated after restore. The app can check the updated value in the afterRestore.

src/java.base/share/classes/java/lang/System.java line 1111:

> 1109:      * a resource and in the {@link javax.crac.Resource#afterRestore(javax.crac.Context) afterRestore method}
> 1110:      * reload environment variables, propagating any change.
> 1111:      *

The constantness of the environment is JDK implementation detail (although likely to be very expected by users). 

It should be enough to say the env is updated along restore, and that app check the update.  But we don't need to force ("The app should ...") to reload values.

src/java.base/share/classes/java/net/InetAddress.java line 197:

> 195:  * @crac This class holds a cache of resolved hostname-address pairs;
> 196:  * this cache is wiped out before checkpoint since this mapping might be
> 197:  * outdated or invalid in the environment where the process is restored.

The description and rationale behind should be separated. "this cache is wiped out before checkpoint, so after the process is restored any lookup causes name address resolution. This ensures addresses are actual in the new environment"?

And I'm not sure rationale part is not evident here.

src/java.base/share/classes/java/security/SecureRandom.java line 366:

> 364:      * the {@link Security#getProviders() Security.getProviders()} method.
> 365:      *
> 366:      * @crac See provider documentation for details of behaviour after restore from a checkpoint.

Shouldn't be a link here?

src/java.base/share/classes/java/util/Timer.java line 328:

> 326:      * could execute many times after a restore. This is likely an undesired
> 327:      * behaviour, therefore it is recommended to cancel the task before
> 328:      * checkpoint and schedule it again after restore.

In this wording this looks like a problem, and I'm not convinced that the problem exists (otherwise we may want probably to do something about that).

Could you change the description to something more neutral? "could execute many times after a restore, catching up as described above. If this is not desirable,  the task can be canceled and scheduled again in a  {@link Resource} implementation"

src/java.base/share/classes/jdk/internal/util/jar/PersistentJarFile.java line 39:

> 37: 
> 38: /**
> 39:  * @crac It is assumed that JAR files opened through this class thatn are open

"thatn" typo

src/java.base/unix/classes/sun/net/www/protocol/jar/JarFileFactory.java line 46:

> 44:  *
> 45:  * @crac All JarFile instances that are not referenced from elsewhere are
> 46:  * removed from the cache before a checkpoint.

Looks OK for a non-public class. But in general, the new tag is used for implementation notes like this and also for the description of the behavior of the internal Resource implementation with more impact on the user. It would be nice to somehow distinguish these two (or more) uses of the tag.

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

PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1165751039
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1165757063
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1165765261
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1170163618
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1170164574
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1170180639
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1170187510
PR Review Comment: https://git.openjdk.org/crac/pull/51#discussion_r1170196932