Skip to content

[JENKINS-68080] DurableTaskStep.HandlerImpl.printStreamDelegate requires --add-opens #212

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

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

jglick
Copy link
Member

@jglick jglick commented Mar 22, 2022

Fixes tests skipped in #209.

printStreamDelegate = FilterOutputStream.class.getDeclaredField("out");
} catch (NoSuchFieldException x) {
// Defined in Java Platform and protected, so should not happen.
throw new ExceptionInInitializerError(x);
}
printStreamDelegate.setAccessible(true);
is the problem

hudson.remoting.ProxyException: java.lang.reflect.InaccessibleObjectException: Unable to make field protected java.io.OutputStream java.io.FilterOutputStream.out accessible: module java.base does not "opens java.io" to unnamed module @… [master]
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354) [master]
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297) [master]
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178) [master]
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172) [master]
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$HandlerImpl.<clinit>(DurableTaskStep.java:702) [master]

--add-opens java.base/java.io=ALL-UNNAMED is passed to the controller, but not the agent by JenkinsRule.createComputerLauncher. Normally the error is swallowed because https://github.com/jenkinsci/durable-task-plugin/blob/0d250a6142908f591bda91d8658ab1b102611ce3/src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java#L548 runs this asynch and Remoting apparently discards errors from asynch tasks without logging them? (act actually works just as well, and exposes the error.)

@jglick jglick requested a review from basil March 22, 2022 01:18
@jglick jglick added the bug label Mar 22, 2022
@jglick jglick requested a review from dwnusbaum March 22, 2022 01:18
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I have been considering whether to add any Add-Opens directives to Remoting's MANIFEST.MF. After some discussion in jenkinsci/docker-agent#207 (comment) @timja and I concluded they may have been needed in the past (perhaps for older versions of XStream?) but do not seem to be needed in the present, so in jenkinsci/docker-inbound-agent#263 I removed them from our Docker images and did not add anything to Remoting's MANIFEST.MF. This would be the first case I am aware of where Add-Opens is required on the agent side. (On the controller, of course, we know it is needed for XStream.) If this is the only such case, and it is only needed for better error handling, then I suppose it is better to keep Remoting's MANIFEST.MF free of any Add-Opens entries. If, on the other hand, we find additional use cases, then it might be worth adding Add-Opens to Remoting's MANIFEST.MF.

@jglick
Copy link
Member Author

jglick commented Mar 22, 2022

Ideally we would not need to use reflection to get the wrapped OutputStream, but the TaskListener interface is built around PrintStream. A bunch of code could be made simpler and more robust (and this fixed) if TaskListener exposed an optional ability to return the underlying OutputStream; I have just not gotten around to it. I agree that adding the JVM flag to agents is probably overkill just to support better error handling in a still-experimental mode.

@car-roll car-roll merged commit 8c259d1 into jenkinsci:master Mar 22, 2022
@jglick jglick deleted the printStreamDelegate-JENKINS-68080 branch March 22, 2022 12:30
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.

3 participants