[crac] RFR: Handle open file descriptors with configurable policies [v9]

[Anton Kozlov]

On Thu, 29 Jun 2023 16:09:58 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> When the application does not close some file descriptors through Resources we can use `jdk.crac.resource-policies` pointing to a configuration file that might adjust the behaviour. The file uses a simple YAML-conformant format with individual rules separated by a line with `---`:
>> 
>> 
>> type: file
>> path: /path/to/*.txt
>> action: close
>> ---
>> type: socket
>> localAddress: 127.0.0.1
>> localPort: 8080
>> action: ignore
>> 
>> 
>> Available types:
>> * `file`: supports `path` with 'glob' pattern matching (see FileSystem.getPathMatcher() for details)
>> * `pipe`: anonymous pipes (named pipe is handled as `file`)
>> * `socket`: can be refined using `family`, `localAddress`, `localPort`, `localPath` (in case of Unix sockets), `remoteAddress`, `remotePort` and `remotePath`
>> 
>> Actions depend on each resource, defaulting to `error`, with common options `close` and `ignore`. Files have `reopen` action, too.
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Cosmetic fixes (imports)

I really like how it evolved! I cannot verify the correctness of handling of the new policies everywhere in the JDK code, but the changes look safe when the new configuration is not set. Just a few more comments.

src/java.base/share/classes/java/io/FileOutputStream.java line 489:

> 487:                 }
> 488:                 // FileOutputStream writes only forward, so it makes sense to reopen it appending
> 489:                 open(path, true);

This should be done depending on the initial mode, and in the non-appending mode, we should also store and restore the position (e.g `getChannel.position()`).

src/java.base/share/classes/java/lang/Process.java line 876:

> 874:                 case "ignore":
> 875:                     if (Boolean.parseBoolean(policy.params.getOrDefault("warn", "false"))) {
> 876:                         LoggerContainer.warn("CRaC: {0} was not closed by the application!", this);

Nit: this is a CRaC-specific logger, do we need to specify `CRaC: ` prefix?

src/java.base/share/classes/jdk/internal/crac/JDKFileResource.java line 46:

> 44: 
> 45:         OpenResourcePolicies.Policy policy = OpenResourcePolicies.findForPath(false, path);
> 46:         String action = policy != null ? policy.action.toLowerCase() : "error";

Policies could be more "typed", e.g. an OpenFilePolicy exposing an enum with predefined actions... But this can be done in follow ups. Looks good for now.

src/java.base/share/classes/jdk/internal/crac/JDKFileResource.java line 52:

> 50:                 // Files on the classpath are considered persistent, exception is not thrown
> 51:                 yield matchClasspath(path) ? NO_EXCEPTION :
> 52:                         () -> new CheckpointOpenFileException(path, getStackTraceHolder());

So error + no exception is a valid combination? Should not we choose `ignore` as the default for files matching classpath instead?

src/java.base/share/classes/jdk/internal/crac/JDKSocketResourceBase.java line 138:

> 136:     public void afterRestore(Context<? extends Resource> context) throws Exception {
> 137:         // Don't do anything when we've already failed
> 138:         if (originalFd < 0 || error) {

The orignalFd seems to be a leftover, it's not clear why is it stored as a part of the state. Should not we just consider `error`?

src/java.base/unix/classes/sun/nio/ch/FileDispatcherImpl.java line 64:

> 62:         // might read configuration files with this or later priority.
> 63:         // It's difficult to trigger static initialization outside the package.
> 64:         Core.Priority.PRE_FILE_DESCRIPTORS.getContext().register(resourceProxy);

The change looks safe but not clear. The resourceProxy eventually calls FileDispatcherImpl.beforeCheckpoint, that closes preClose sockets, blocking FDI.preClose() operation. Does the problem appear when a configuration file is closed? Or is it just registration to NORMAL priority when it is being processed already?

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

PR Review: https://git.openjdk.org/crac/pull/69#pullrequestreview-1511351580
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1250995673
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1251009152
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1251123379
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1251126488
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1251131308
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1251109204