Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8324668: JDWP process management needs more efficient file descriptor handling #17588

Closed
wants to merge 10 commits into from
+109 −15
Closed

8324668: JDWP process management needs more efficient file descriptor handling #17588

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6508c95
8324668: JDWP process management needs more efficient file descriptor…
jaikiran Jan 26, 2024
4a1155b
use the right include for rlim_t - <sys/resource.h>
jaikiran Jan 26, 2024
082cd9b
ignore return values, cast to void
jaikiran Jan 27, 2024
af2472d
log a warning message if we fallback to slower logic of closing file …
jaikiran Jan 27, 2024
03288cf
David's review - use standard isdigit instead of custom isAsciiDigit
jaikiran Jan 29, 2024
6add9f9
improve code comments
jaikiran Jan 29, 2024
e4a8573
add a log message to help debuggability
jaikiran Jan 29, 2024
c1decbf
assert that we don't pass values higher than INT_MAX to close()
jaikiran Jan 30, 2024
3d323f7
Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
jaikiran Jan 31, 2024
afef982
Minor - fix alignment
jaikiran Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
View file
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@
#include <unistd.h>
#include <string.h>
#include <ctype.h>
#include "log_messages.h"
#include "sys.h"
#include "util.h"

@@ -105,11 +106,13 @@ closeDescriptors(void)
while ((dirp = readdir(dp)) != NULL) {
int fd;
if (isAsciiDigit(dirp->d_name[0]) &&
(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd)
close(fd);
(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd) {
(void)close(fd);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really, really like to check close() for errors and report any.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. If we are seeing issues with close(), probably it is best to fail. That way we'll get a bug report and can figure out exactly why it is failing rather than blindly proceeding without having properly closed all fds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to check close for errors then we will have to know for certain we are only trying to close open fd's, and we will have to deal with EINTR. I would think this is a "best effort" to close open FD's and not something we have to care about in detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to check close for errors then we will have to know for certain we are only trying to close open fd's, and we will have to deal with EINTR. I would think this is a "best effort" to close open FD's and not something we have to care about in detail.

Right, EINTR would need to be ignored as it would be wrong to restart. I'm scratching my head as to why this code would even do with EIO or if it even possible here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at man close, EIO gets returned if:

[EIO] A previously-uncommitted write(2) encountered an input/output error.

If I understand this launch=<cmd> option correctly for the jdwp agent, then the arbitrary command gets launched very early in the lifecycle of the JVM. So in theory, I think there shouldn't be too many file descriptors that the JVM has opened so far and the chances of it having started writing to some of those opened file descriptors (other than standard out/err) perhaps is slim? But that's just a guess. Probably not a strong one too, given that if the JVM was launched with any other agent along with jdwp, then it's unknown if the other agent would have opened any file descriptors for write.

My understanding of these close() calls after fork() in this code, matches David's - I think it's a best effort attempt. So perhaps ignoring such failures, like we do now, is OK?

Copy link

@gerard-ziemski gerard-ziemski Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring errors just doesn't seem like a good strategy (especially in light of so many mysterious timeouts that are plaguing MACH5 tests mostly on macOS)

This fix right here that we are doing doesn't have to deal with making this code more robust, as long as we follow up in another issue. For that then, I filed JDK-8324979, and that frees us up to move on with this fix.

}

}

closedir(dp);
(void)closedir(dp);

return 1; // success
}
@@ -130,6 +133,9 @@ forkedChildProcess(const char *file, char *const argv[])
JDI_ASSERT(max_fd != (rlim_t)-1);
/* leave out standard input/output/error file descriptors */
rlim_t i = STDERR_FILENO + 1;
LOG_MISC(("warning: failed to close file descriptors of"
" child process optimally, falling back to closing"
" %d file descriptors sequentially", (max_fd - i + 1)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is ever going to see this. This logging is off by default, and when turned on goes to the logfile, not stdout or stderr. Perhaps ERROR_MESSAGE() would be better, which goes to stderr and also gives the option of aborting if the debug agent is launched with errorexit=y.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Chris for pointing to ERROR_MESSAGE(). I've updated the PR to use that.

for (; i < max_fd; i++) {
(void)close(i);
Copy link

@gerard-ziemski gerard-ziemski Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, I'd really like to check for close()for errors here and report any.

And if we find errors here, do we really want to proceed knowing that something went wrong already?

}