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

[JENKINS-69647] Eliminate reflection from WinProcess #71

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

basil
Copy link
Member

@basil basil commented Sep 20, 2022

Problem

See JENKINS-69647. Stopping a build on Java 17 results in:

ERROR: Failed to join a process
java.lang.reflect.InaccessibleObjectException: Unable to make field private final long java.lang.ProcessImpl.handle accessible: module java.base does not "opens java.lang" to unnamed module @5908db45
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(Unknown Source)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(Unknown Source)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Unknown Source)
	at java.base/java.lang.reflect.Field.setAccessible(Unknown Source)
	at org.jvnet.winp.WinProcess.<init>(WinProcess.java:44)
	at hudson.util.ProcessTree$Windows.get(ProcessTree.java:696)
	at hudson.util.ProcessTree.killAll(ProcessTree.java:182)
	at hudson.Proc$LocalProc.destroy(Proc.java:390)
	at hudson.Proc$LocalProc.kill(Proc.java:382)
	at hudson.Proc$1.run(Proc.java:167)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source)
	at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at java.base/java.lang.Thread.run(Unknown Source)

Evaluation

The use of reflection is unnecessary as of Java 9+, which natively supports getting the process ID through Process#pid. Furthermore, the use of reflection is actively harmful on Java 17, causing the above error.

Solution

Eliminate reflection in favor of native Java Platform functionality.

Implementation

This PR does the following:

  • Complete the transition of this plugin to the Jenkins project by migrating from Kohsuke's parent POM to the Jenkins parent POM, thereby requiring Java 11 or newer and dropping support for publication to Maven Central.
  • Adjust the code to use native Java Platform process functionality wherever possible rather than reflection or custom code.
  • Add a Jenkinsfile for the Java (not native) portions of the build.

This PR explicitly does not cover any of the following:

  • Add support for publication to Maven Central using the Jenkins parent POM.
  • Revive CI (originally done with AppVeyor, now broken) for the native (not Java) portions of the build.
  • Remove the (now-unused) Native#getProcessId(long), recompile the native binaries, and re-sign them.

The reason for explicitly marking these tasks out of scope is that I am not interested in doing them at the present time. I am not against somebody else doing them, but they are not necessary in the short to medium term and I am explicitly not volunteering to do them myself. Once approved, I plan to merge this PR and do a release to the Jenkins Artifactory server, with the release notes mentioning that this release (a) requires Java 11 or newer and (b) is now published on repo.jenkins-ci.org rather than Maven Central. Note that we have been following the same process for other libraries adopted from Kohsuke, including lib-mock-javamail and lib-file-leak-detector, and while in some cases users have asked for Maven Central releases, so far nobody from the community has stepped up to perform releases on Maven Central. Similarly, I do not intend to maintain a Java 8 support branch for users outside of the Jenkins project who want to continue to use Java 8. As above, I am not against somebody stepping up to do this, but I am explicitly not volunteering to do it myself.

Testing done

build.cmd cleanbuild passes on Windows.

@basil basil merged commit 7c790fd into jenkinsci:master Sep 22, 2022
@basil basil deleted the JENKINS-69647 branch September 22, 2022 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants