[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)

First batch of the comments

src/java.base/share/classes/java/io/FileDescriptor.java line 75:

> 73:                     claimedFDs.claimFd(self, self, null);
> 74:                 }
> 75: 

Can we have a policy for a FileDescriptor to prevent it from throwing an exception? Suppose we have an issue that a FileDescriptor throws due to a race, e.g. a containing object referring to an FD was GCed, FD is about to be GCed, but still alive, so FD will throw an exception. It will be useful to suppress the exception by the configuration, although of course, it's discouraged.

src/java.base/share/classes/java/io/FileDescriptor.java line 76:

> 74:                 }
> 75: 
> 76:                 claimedFDs.claimFd(self, self, () -> new CheckpointOpenResourceException(

Apparently the second claim is expected to do nothing. It will be useful highight that, e.g. by 

} else {
  claimedFDs.claimFd(self, self, () -> new CheckpointOpenResourceException(
  ...

src/java.base/share/classes/jdk/internal/crac/JDKFdResource.java line 10:

> 8: 
> 9: public abstract class JDKFdResource implements JDKResource {
> 10:     public static final String COLLECT_FD_STACKTRACES_PROPERTY = "jdk.crac.collect-fd-stacktraces";

AFAICS, the only reason it is not private anymore is the tests.

OK, seems fine, as we consider this is a part of the user interface.

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

> 210:         // the FD value set breaks JDKSocketResource (we don't want the extra
> 211:         // test if the FD resource has been marked).
> 212:         fdAccess.closeNoCleanup(fd);

Should not it be just `close()`ed? `unregisterCleanup` looks to be idempotent. And I remember you had a mail thread regarding this close/close0, could you remind the link?

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

PR Review: https://git.openjdk.org/crac/pull/69#pullrequestreview-1507100500
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1250948089
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1247807201
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1247833826
PR Review Comment: https://git.openjdk.org/crac/pull/69#discussion_r1247845886