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.

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

Make DaemonRegistry.getProcessId0 more robust #612

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

jglick
Copy link
Contributor

@jglick jglick commented Mar 22, 2022

Fixes #608. Tested with Java 11 in non-native mode (from ./dist/target/mvnd-0.7.2-SNAPSHOT-linux-amd64/bin/) on Linux and Windows 10.

if (pid == null) {
pid = ManagementFactory.getRuntimeMXBean().getName().split("@", 0)[0];
}
if (pid == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this branch was impossible previously (pid would have been set above), so

int rpid = new Random().nextInt(1 << 16);
LOGGER.warn("Unable to determine PID, picked a random number=" + rpid);
return rpid;
dating back to e3b58e8 was dead code?

final Path self = Paths.get("/proc/self");
if (Files.exists(self)) {
pid = self.toRealPath().getFileName().toString();
if (Os.current() == Os.LINUX) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I originally thought if (Os.current() != Os.WINDOWS && Os.current() != Os.MAC) would be better, because both UNKNOWN or any future expansion of the Os enum are likely to be Unix flavors implementing procfs. But I admit that assumption is rather weak and your proposal isn't actually bad because if ManagementFactory.getRuntimeMXBean().getName().split() does not work for some UNKNOWN or future Os, then somebody would complain and we can make the condition more specific. Thus 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. https://en.wikipedia.org/wiki/Procfs claims it was removed from OpenBSD. Anyway IIUC this code can be simplified when dropping Java 8 support? (Or perhaps now, if you use reflection?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, ProcessHandle.current().pid() would simplify it a lot. I'd vote for using reflection as a fallback only if we have real world reports that the current code does not work on some specific platform.

@ppalaga ppalaga merged commit 2ab4cd8 into apache:master Mar 22, 2022
@ppalaga
Copy link
Contributor

ppalaga commented Mar 22, 2022

Thanks for your contribution, @jglick!

@jglick jglick deleted the getProcessId0 branch March 23, 2022 12:59
@gnodet gnodet added this to the 0.7.2 milestone Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mvnd on Windows throws java.lang.NumberFormatException: For input string: "self"
3 participants