RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v3]

[David Holmes]

On Sat, 27 Jan 2024 13:09:45 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to address https://bugs.openjdk.org/browse/JDK-8324668?
>> 
>> This change proposes to fix the issue in jdwp where when launching a child process (for the `launch=<cmd>` option), it iterates over an extremely large number of file descriptors to close each one of those. Details about the issue and the proposed fixed are added in a subsequent comment in this PR https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, tier2 and tier3 testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - log a warning message if we fallback to slower logic of closing file descriptors
>  - ignore return values, cast to void

Thanks for taking this on @jaikiran and for determining the underlying problem!

Seems a reasonable approach to copy. A few nits and a suggestion.

Thanks.

src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 52:

> 50: 
> 51: int
> 52: isAsciiDigit(char c)

The standard `isdigit` function from `ctype.h` does this.

src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 75:

> 73: // input/output/error file descriptors will not be closed
> 74: // by this function. this function returns 0 on failure
> 75: // and 1 on success

Pre-existing nit: Please start sentences with a capital and end with a period. Thanks. This applies through the copied code.

src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 103:

> 101: #endif
> 102: 
> 103:     if ((dp = opendir(FD_DIR)) == NULL)

It is better to log the failure here, where you can actually report the reason.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1847815765
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469067372
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469067771
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469069319