Use PowerShell instead of WMIC for detecting zombie process on Windows#3258
Use PowerShell instead of WMIC for detecting zombie process on Windows#3258olamy merged 2 commits intoapache:masterfrom
Conversation
WMIC is not available in recent Windows versions. The new solution with PowerShell should work for Windows 8 and newer and Windows Server 2012 and newer. The change is deliberately as small as possible, leaving most of the old WMIC handling in place because there is a plan to deprecate this class anyway. On performance note: executing the check via PowerShell is notably slower than with WMIC (low hundreds of millis vs tens of millis). However, since this check is used only to detect zombie process and only once per tens of seconds, this will hopefully be good enough. Much better and faster implementation could be done once the minimal supported Java moves to 9 or later via ProcessHandle, until then this should do.
| assumeTrue(new File(System.getenv("SystemRoot"), "System32\\Wbem").isDirectory()); | ||
| assumeTrue(new File(System.getenv("SystemRoot"), "System32\\Wbem\\wmic.exe").isFile()); | ||
| assertThat((Boolean) invokeMethod(PpidChecker.class, "hasWmicStandardSystemPath")) | ||
| assumeTrue(new File(System.getenv("SystemRoot"), "System32\\WindowsPowerShell\\v1.0").isDirectory()); |
There was a problem hiding this comment.
(not a windows user) is this version number something stable? (this looks like something might change in the future with upgrade etc...)
There was a problem hiding this comment.
Some old PowerShell versions have been removed recently. We need to take that into account.
There was a problem hiding this comment.
not a regular user myself but all googling suggests the location is always the same even if the actual powershell version inside is different, probably to keep compatibility.
at least one MS reference
https://learn.microsoft.com/en-us/windows/msix/psf/run-scripts-with-package-support-framework
64-bit computer:
64-bit executable: %SystemRoot%\system32\WindowsPowerShell\v1.0\powershell.exe
32-bit executable: %SystemRoot%\SysWOW64\WindowsPowerShell\v1.0\powershell.exe
32-bit computer:
32-bit executable: %SystemRoot%\system32\WindowsPowerShell\v1.0\powershell.exe
There was a problem hiding this comment.
BTW the implementation should work even if powershell is in different location - the same way the WMIC was able to be used from elsewhere if it was found on the PATH. It just prefers this standard location to be used first.
The test itself also doesn't fail if the location is not available but other tests will fail if no powershell is found, again the same as with previous WMIC implementation.
There was a problem hiding this comment.
Reasonable, maybe a comment should be added...
| !surefire-its/target/ConsoleOutputIT_*/target/surefire-reports/*-jvmRun*-events.bin | ||
| timeout-minutes: 600 | ||
| os-matrix: '[ "ubuntu-latest", "windows-2022", "macos-latest" ]' | ||
| os-matrix: '[ "ubuntu-latest", "windows-latest", "macos-latest" ]' |
There was a problem hiding this comment.
do we want to keep specific versions of windows and maybe add latest too?
There was a problem hiding this comment.
I guess that is a question for someone maintaining the project. If it is acceptable to have bigger CI matrix, why not.
BTW this is already being discussed at #3252, we could try to keep this PR as simple as possible.
There was a problem hiding this comment.
And GHA usage is monitored (eventually capped) at org level ("apache") so we do not really want increase too much the usage.
fix for #3176
WMIC is not available in recent Windows versions.
The new solution with PowerShell should work for Windows 8 and newer
and Windows Server 2012 and newer.
The change is deliberately as small as possible, leaving most
of the old WMIC handling in place because there is a plan to deprecate
this class anyway (#3252).
On performance note: executing the check via PowerShell is notably
slower than with WMIC (low hundreds of millis vs tens of millis).
However, since this check is used only to detect zombie process
and only once per tens of seconds, this will hopefully be good enough.
Much better and faster implementation could be done via ProcessHandle once the minimal
supported Java moves to 9 or later. There is already a work in progress at #3252 for this, after which only combinations with Java 8 and Windows will use PowerShell for doing the check. Until then this should do.
Since windows-2022 was hardcoded precisely because of missing WMIC on newer windows, I propose to change the matrix too. I don't think we need to increase number of tested combinations so just replace windows-2022 with windows-latest.
CI run with windows-2022 available at https://github.com/jbliznak/maven-surefire/actions/runs/21788605653
CI run with windows-latest https://github.com/jbliznak/maven-surefire/actions/runs/21788596928
Following this checklist to help us incorporate your
contribution quickly and easily:
mvn clean installto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean install).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.