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

API for process arguments and environment #1654

Merged
merged 52 commits into from Jul 18, 2021
Merged

API for process arguments and environment #1654

merged 52 commits into from Jul 18, 2021

Conversation

basil
Copy link
Contributor

@basil basil commented Jun 5, 2021

Fixes #369. Posting this draft for initial feedback on the API and the overall strategy. The Linux implementation is shamelessly copied from the (MIT-licensed) code in Jenkins core. If this approach of moving code from Jenkins core into this project is deemed suitable, I'll look into moving over the Jenkins implementations for macOS, AIX, and Solaris and writing new implementations for the BSDs.

@basil basil marked this pull request as draft June 5, 2021 03:00
@dbwiddis dbwiddis requested review from dbwiddis and mprins June 5, 2021 04:43
@dbwiddis
Copy link
Member

dbwiddis commented Jun 5, 2021

Thanks for starting this contribution! I think this will be a great addition. I'm not sure why I decided it was out of scope earlier, perhaps because of the Windows implementation needing to dig into process memory, something I wasn't about to attempt based on a few snippets in a Stack Overflow answer.

I like the addition and the new proposed API looks reasonable (I've tagged @mprins for review to offer an opinion on the API as well.) Some general thoughts after a brief look at the API:

  • Later we'll have to be more verbose in the javadocs with caveats, "best effort" notation to advise users what they may need to double-check, etc.
  • Whether it was a good decision or bad one, I don't have OSHI throw exceptions, so the "unimplemented" ones should just return empty lists/maps. Use a default method in the interface and then you don't need all the different OS implementations that throw UnsupportedOperationException.
  • I don't think it's necessary to make a freshly-generated list or map from a static method unmodifiable. If each method call will produce a new copy, there's no harm in handing the user a mutable structure.

With regard to using code from Jenkins. I want to respect the intent of the MIT license which requires the copyright notice to accompany "copies" or "substantial portions". I hand-wave that at OSHI by referring to the "Contributors", which credits authors via the git history. But that will show up as your commit, not the original author. That said, I don't think it's an issue for the code I see so far, only the parsing routine looks "copied" and that's pretty much the only way to do it. I think I've seen similar C code in the JDK's ProcessHandle implementation. Would be nice to give attribution to the original author in a comment, though.

For the AIX port, do you have access to an AIX machine? I got access to a server at Polarhome when I did the AIX port here, but it's been down for several months so I have no means to develop or test code there.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Some specific code suggestions in addition to/clarifying my comments above.

Copy link
Member

@mprins mprins left a comment

Choose a reason for hiding this comment

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

shouldn't be too hard to get program arguments on OpenBSD, not sure about program environment, either one may need elevated user privileges for security reasons

@coveralls
Copy link

coveralls commented Jun 26, 2021

Coverage Status

Coverage decreased (-0.05%) to 85.914% when pulling 5420026 on basil:env into 9ae88cc on oshi:master.

@dbwiddis
Copy link
Member

I went ahead and cleaned up the API, Linux implementation, some utils, and did the macOS implementation (we already were fetching the bytes, just needed to put them in a map).

I'm still uncomfortable with the Windows implementation(s) as they all involve API functions which are stated may be removed in future versions of Windows, or are unreliable involving race conditions. And I think require running as administrator.

Are you going to try to put in the Solaris and AIX ports?

@dbwiddis
Copy link
Member

Good News! I got the Windows version working. As a bonus, the same memory that provides the environment pointer also gets us CWD strings (that we previously didn't support) and Command Line strings (without the overhead of WMI).

    public static void main(String[] args) {
        SystemInfo si = new SystemInfo();
        OperatingSystem os = si.getOperatingSystem();
        OSProcess p = os.getProcess(os.getProcessId());
        System.out.println("CWD: " + p.getCurrentWorkingDirectory());
        System.out.println("CMD: " + p.getCommandLine());
        System.out.println("ARG: " + p.getArguments());
        System.out.println("ENV: " + p.getEnvironmentVariables());
    }

Output:

CWD: Z:\git\oshi\oshi-core\
CMD: "C:\Program Files\Java\jre1.8.0_291\bin\javaw.exe" -Dfile.encoding=UTF-8 -classpath "C:\Program Files\Java\jre1.8.0_291\lib\resources.jar;C:\Program Files\Java\jre1.8.0_291\lib\rt.jar;C:\Program Files\Java\jre1.8.0_291\lib\jsse.jar;C:\Program Files\Java\jre1.8.0_291\lib\jce.jar;C:\Program Files\Java\jre1.8.0_291\lib\charsets.jar;C:\Program Files\Java\jre1.8.0_291\lib\jfr.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\access-bridge.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\cldrdata.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\dnsns.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\jaccess.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\jfxrt.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\localedata.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\nashorn.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunec.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunjce_provider.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunmscapi.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunpkcs11.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\zipfs.jar;Z:\git\oshi\oshi-core\target\classes;C:\eclipse\plugins\org.junit.jupiter.api_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.engine_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.migrationsupport_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.params_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.commons_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.engine_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.launcher_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.runner_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.suite.api_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.vintage.engine_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.opentest4j_1.0.0.v20180327-1502.jar;C:\eclipse\plugins\org.apiguardian_1.0.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit_4.12.0.v201504281640\junit.jar;C:\eclipse\plugins\org.hamcrest.core_1.3.0.v20180420-1519.jar;C:\Users\dbwiddis\.m2\repository\net\java\dev\jna\jna\5.8.0\jna-5.8.0.jar;C:\Users\dbwiddis\.m2\repository\net\java\dev\jna\jna-platform\5.8.0\jna-platform-5.8.0.jar;C:\Users\dbwiddis\.m2\repository\org\slf4j\slf4j-api\1.7.30\slf4j-api-1.7.30.jar" oshi.software.os.windows.WindowsOSProcess
ARG: [C:\Program Files\Java\jre1.8.0_291\bin\javaw.exe, -Dfile.encoding=UTF-8, -classpath, C:\Program Files\Java\jre1.8.0_291\lib\resources.jar;C:\Program Files\Java\jre1.8.0_291\lib\rt.jar;C:\Program Files\Java\jre1.8.0_291\lib\jsse.jar;C:\Program Files\Java\jre1.8.0_291\lib\jce.jar;C:\Program Files\Java\jre1.8.0_291\lib\charsets.jar;C:\Program Files\Java\jre1.8.0_291\lib\jfr.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\access-bridge.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\cldrdata.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\dnsns.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\jaccess.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\jfxrt.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\localedata.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\nashorn.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunec.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunjce_provider.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunmscapi.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\sunpkcs11.jar;C:\Program Files\Java\jre1.8.0_291\lib\ext\zipfs.jar;Z:\git\oshi\oshi-core\target\classes;C:\eclipse\plugins\org.junit.jupiter.api_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.engine_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.migrationsupport_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.jupiter.params_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.commons_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.engine_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.launcher_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.runner_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.platform.suite.api_1.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit.vintage.engine_5.1.0.v20180327-1502.jar;C:\eclipse\plugins\org.opentest4j_1.0.0.v20180327-1502.jar;C:\eclipse\plugins\org.apiguardian_1.0.0.v20180327-1502.jar;C:\eclipse\plugins\org.junit_4.12.0.v201504281640\junit.jar;C:\eclipse\plugins\org.hamcrest.core_1.3.0.v20180420-1519.jar;C:\Users\dbwiddis\.m2\repository\net\java\dev\jna\jna\5.8.0\jna-5.8.0.jar;C:\Users\dbwiddis\.m2\repository\net\java\dev\jna\jna-platform\5.8.0\jna-platform-5.8.0.jar;C:\Users\dbwiddis\.m2\repository\org\slf4j\slf4j-api\1.7.30\slf4j-api-1.7.30.jar, oshi.software.os.windows.WindowsOSProcess]
ENV: {LOCALAPPDATA=C:\Users\dbwiddis\AppData\Local, PROCESSOR_LEVEL=6, FP_NO_HOST_CHECK=NO, USERDOMAIN=DWA909, LOGONSERVER=\\DWA909, JAVA_HOME=C:\Program Files\Java\jre1.8.0_201\bin\javaw.exe, SESSIONNAME=Console, ALLUSERSPROFILE=C:\ProgramData, PROCESSOR_ARCHITECTURE=x86, PSModulePath=C:\Windows\system32\WindowsPowerShell\v1.0\Modules\, SystemDrive=C:, APPDATA=C:\Users\dbwiddis\AppData\Roaming, USERNAME=dbwiddis, CommonProgramFiles=C:\Program Files\Common Files, Path=C:/Program Files/Java/jre1.8.0_291/bin/client;C:/Program Files/Java/jre1.8.0_291/bin;C:/Program Files/Java/jre1.8.0_291/lib/i386;C:\Program Files\Parallels\Parallels Tools\Applications;C:\Program Files\Common Files\Oracle\Java\javapath;C:\ProgramData\Oracle\Java\javapath;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\eclipse;, PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC, OS=Windows_NT, COMPUTERNAME=DWA909, PROCESSOR_REVISION=9e0d, ComSpec=C:\Windows\system32\cmd.exe, ProgramData=C:\ProgramData, HOMEPATH=\Users\dbwiddis, SystemRoot=C:\Windows, TEMP=C:\Users\dbwiddis\AppData\Local\Temp, HOMEDRIVE=C:, PROCESSOR_IDENTIFIER=x86 Family 6 Model 158 Stepping 13, GenuineIntel, USERPROFILE=C:\Users\dbwiddis, TMP=C:\Users\dbwiddis\AppData\Local\Temp, ProgramFiles=C:\Program Files, PUBLIC=C:\Users\Public, NUMBER_OF_PROCESSORS=8, windir=C:\Windows}

Still a work in progress as I haven't handled the IsX64/IsWow checks, but this definitely looks doable.

dbwiddis and others added 2 commits June 27, 2021 13:01
@basil
Copy link
Contributor Author

basil commented Jun 27, 2021

This looks like it's coming along nicely! I'll try to keep helping to chip away at this as I have time.

The use of CommandLineToArgvW in the Windows implementation is very nice! Jenkins has its own quoted string tokenizer that splits apart the single string into an approximated tokenization. Using a native Windows API to do this seems cleaner.

I added a FreeBSD implementation for OSHI. I tested it on a 64-bit FreeBSD system running in a 64-bit Java process and inspecting both 64-bit and 32-bit processes. I think the OpenBSD implementation for OSHI will be similar modulo differences in the sysctl invocations (see OpenBSD's documentation).

For AIX and Solaris, I still think the easiest path forward is to copy the implementation from Jenkins (with proper attribution, of course). I have an illumos box to test on, but unfortunately I don't have an AIX box. On a side note, I don't like how the existing AIX and Solaris implementation in OSHI invokes ps(1) to get the list of all processes. These are procfs-based Unix platforms, so we should be able to more efficiently get the list of processes from /proc the same way we do for Linux in OSHI. Jenkins does it this way for Linux, AIX, and Solaris. While this work isn't strictly required for implementing this new feature, it is closely related. I think we might as well do that in this PR, if we're going to be testing this anyway. (This also gets me closer to my personal goal of using OSHI from Jenkins and getting Jenkins out of the process handling business.)

Similarly, I don't like how the existing FreeBSD implementation in OSHI invokes ps(1) to get the list of all processes. There is a kern.proc.all sysctl available for this. Jenkins has been using the kern.proc.all sysctl for macOS for a while, and I recently wrote and tested a FreeBSD implementation of the kern.proc.all sysctl for Jenkins (which was almost identical to the macOS implementation in Jenkins). I imagine it would be similar for OpenBSD. I think that using the kern.proc.all sysctl in OSHI for FreeBSD and OpenBSD (and possibly macOS?) might be desirable. Not only is it more efficient than ps(1), but it would also match the implementation in Jenkins. However, the kern.proc.all sysctl API is a bit awkward, as you have to call it twice: once to get the size of the buffer you need and once to get the actual data. In between, the size required might have changed (for example, if new processes were spawned), so you have to do an awkward dance of increasing the buffer size and retrying. Still, it's better than ps(1), which requires the expensive creation of a new process. While this work isn't strictly required for implementing this new feature, it is closely related. I think we might as well do that in this PR, if we're going to be testing this anyway. (This also gets me closer to my personal goal of using OSHI from Jenkins and getting Jenkins out of the process handling business.)

I noticed that Java 9+ has ProcessHandle and ProcessHandle.Info classes that provides much of the information needed to implement OperatingSystem and OSProcess (but not the environment variables, unfortunately). Perhaps on the Java 11 branch, we'd want to try the Java Platform's implementation and fall back to our own implementation only if that doesn't work (or vice versa).

Another question: Should we try to preserve the order of the environment variable entries by using LinkedHashMap rather than HashMap? I think it would be nice to preserve the order of entries returned by the OS in the map that we return, but I don't think we should commit to any particular order in the API or that consumers should rely on any particular order being returned.

@dbwiddis
Copy link
Member

I have an illumos box to test on, but unfortunately I don't have an AIX box.

I have an account on a Polarhome AIX server that I'm happy to share the login for. It's an older version but should be sufficient for this work. Unfortunately the drive failed and it's been down since March. Hoping it comes up soon!

we should be able to more efficiently get the list of processes from /proc the same way we do for Linux in OSHI

There are key differences in access available to a non-root user. I chose the ps approach primarily because it allows that access (owned by root but run by normal users) and, frankly, there's the question of how much work I want to put in to support an OS that few people use. I've discovered crashing bugs in Solaris that were there for 2 years that imply nobody is using OSHI on Solaris. (Heck, even OpenJDK isn't supporting it in the future.).

That doesn't mean I wouldn't love improved code, of course... it's just something I would prefer users of those OS's provide. My goals when porting to other OS's have been "provide the information and don't crash" without too much time spent on performance optimization. See also #1366 discussing exactly what you're talking about for AIX.

I think we might as well do that in this PR, if we're going to be testing this anyway.

I think I'd like to keep them separate for now, so this PR doesn't become overly broad, and breaking changes are more easily found and isolated to a single commit. We certainly can target the same release, and we can design the implementations to be overlapping to ease transition later.

I noticed that Java 9+ has ProcessHandle and ProcessHandle.Info classes that provides much of the information needed to implement OperatingSystem and OSProcess (but not the environment variables, unfortunately). Perhaps on the Java 11 branch, we'd want to try the Java Platform's implementation and fall back to our own implementation only if that doesn't work (or vice versa).

But it's also missing a lot. I seriously considered just using that code in the java11 branch but there were too many gaps. By the time I account for all the fallbacks it just adds to code bloat. Sure, it's sufficient if you want fewer bits of information, but then... the users themselves have the ability to use the ProcessHandle API and not use OSHI. So I don't see any harm in keeping the implementation separate.

I've used the JDK code as inspiration for JNA application of the same underlying code, though, so I don't think we're missing too much.

Longer term (actually mid-term now that JDK17 is EA) I plan on a whole new OSHI artifact built from the ground up leveraging lessons learned. While the current version tries to provide pretty much everything to everyone, the new version will be more limited in scope (CPU, Memory, Processes to start, with community-driven additions after that), use FFI instead of JNA, and put performance at a premium.

Another question: Should we try to preserve the order of the environment variable entries by using LinkedHashMap rather than HashMap? I think it would be nice to preserve the order of entries returned by the OS in the map that we return, but I don't think we should commit to any particular order in the API or that consumers should rely on any particular order being returned.

Entirely reasonable and I'll update the ParseUtil methods to do that as part of my next Windows commit. I realized that need just now when trying to efficiently figure out how to conditionally remove the first environment variable which in my Windows tests produces the string "=::=::" (empty key, and value "::=::" in the map. Other code I've seen checks for any of several special characters in the first key/value pair and it'd be nice to just check one.

For AIX and Solaris, I still think the easiest path forward is to copy the implementation from Jenkins (with proper attribution, of course).

Alternately, I can look at the implementation, understand it, and write it myself. That's what I did with the Linux parsing routines and I think mine are slightly more efficient (less object creation, not that a few nanoseconds will matter). Really the key thing to know is "where is this information stored". Not that I particularly want to avoid copying/attribution, happy to do that if it's truly a drop-in-place, but I'm guessing there's going to be enough tweaking to call it original/inspired by rather than derivative.

@basil
Copy link
Contributor Author

basil commented Jun 28, 2021

Frankly, there's the question of how much work I want to put in to support an OS that few people use. I've discovered crashing bugs in Solaris that were there for 2 years that imply nobody is using OSHI on Solaris. (Heck, even OpenJDK isn't supporting it in the future.).

While I can't speak to AIX, the Solaris legacy is alive and well in the illumos community. While my company has switched from illumos to Linux and left the illumos community, I still have access to (somewhat dated) illumos boxes. And when Jenkins tried to drop Solaris-based OS support, members of the illumos community (working at several different companies) showed up to ask that it be kept. (Many of them use Jenkins to build illumos.) Those same members of the illumos community are also working with the OpenJDK community to keep illumos support alive and well in OpenJDK. My point is that demand for illumos support still exists. In a hypothetical future where Jenkins used OSHI, you'd inherit that installed user base.

My goals when porting to other OS's have been "provide the information and don't crash" without too much time spent on performance optimization.

That is perfectly understandable. But my eventual goal is "get Jenkins out of the process business" without causing regressions for any existing Jenkins users. And Jenkins already uses /proc for AIX and Solaris, so I think using ps(1) would be at least a minor performance regression. I'll have to look into your point about root vs unprivileged access on illumos.

Longer term (actually mid-term now that JDK17 is EA) I plan on a whole new OSHI artifact built from the ground up leveraging lessons learned.

A more explicit error handling and exception system might also be worth considering, if you haven't considered it already. Jenkins still supports Java 8, but the project will probably drop support for Java 8 sometime in the next year or so and start requiring Java 11. Jenkins has relatively simple needs: it needs to be able to list processes (including descendants) and to get arguments and environment variables. A slimmer artifact would increase the likelihood that the Jenkins core team would accept a move to OSHI. Out of curiosity, were you planning on replacing JNA with JNR?

# Conflicts:
#	oshi-core/src/main/java/oshi/software/os/unix/freebsd/FreeBsdOSProcess.java
#	oshi-core/src/main/java/oshi/software/os/unix/openbsd/OpenBsdOSProcess.java
#	oshi-core/src/main/java/oshi/util/ParseUtil.java
#	oshi-core/src/test/java/oshi/util/ParseUtilTest.java
@dbwiddis dbwiddis marked this pull request as ready for review July 11, 2021 22:45
@dbwiddis
Copy link
Member

AIX and Solaris done.

Have no way to test AIX but the implementation matches Solaris so I'm confident it will work.

Any chance you can test Solaris yourself, @basil?

@dbwiddis
Copy link
Member

OK, so the Solaris code works fine on the owning process but fails gloriously on various permissions errors since I haven't bothered to check any return codes. And I'm going to test a more efficient way of reading the bytes.

@dbwiddis dbwiddis marked this pull request as draft July 13, 2021 16:19
@dbwiddis
Copy link
Member

Status report: Solaris code is coming along nicely and is almost in its final state. Still need to figure out something going on with the pointer sizes. They don't always seem to match Native.POINTER_SIZE (e.g., under sudo I seem to get 4-byte offset values). I'm not sure if it's JVM bitness (does sudo run a 32-bit java?) or process data model. I think I can just do the math (evnp-argv)/(argc+1) to back into it, though.

After I get solaris working I'll do the same updates for AIX and see if there's anywhere I can reduce duplication.

@dbwiddis dbwiddis marked this pull request as ready for review July 13, 2021 23:17
@dbwiddis
Copy link
Member

Ready for final review.
I've tested thoroughly on macOS and Windows (x86 7 and x64 10).
@basil please test on Solaris and FreeBSD.
@mprins (optional) if you can test on OpenBSD it'd be helpful. I did on an OpenBSD VM.
@tausiflife (optional) another tester on Linux won't hurt :)

I plan to cut a 5.8.0 release this weekend.

@dbwiddis dbwiddis merged commit 84a9afc into oshi:master Jul 18, 2021
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.

API to read a process' environment variables?
4 participants