[JENKINS-68092] Serialization of `java.util.concurrent` data structure in Pipeline: Groovy #518

basil · 2022-03-22T04:39:48Z

Steps to reproduce

On Java 17, run mvn clean verify -Dtest=org.jenkinsci.plugins.workflow.cps.CpsBodyExecutionTest,org.jenkinsci.plugins.workflow.cps.CpsThreadDumpActionTest -Djenkins.version=2.339 -Denforcer.skip -Djenkins-test-harness.version=1723.vcd938b_e66072 '-DargLine=--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED'

Expected results

Note: These are the actual results when adding --add-opens java.base/java.util.concurrent=ALL-UNNAMED to the above arguments:

The test passes.

Actual results

The test fails with:

com.thoughtworks.xstream.converters.ConversionException: 
No converter available
             - Debugging information ----
message             : No converter available
type                : java.util.concurrent.ConcurrentSkipListMap
converter           : com.thoughtworks.xstream.converters.reflection.SerializableConverter
message<1>          : Unable to make private void java.util.concurrent.ConcurrentSkipListMap.readObject(java.io.ObjectInputStream) throws java.io.IOException,java.lang.ClassNotFoundException accessible: module java.base does not "opens java.util.concurrent" to unnamed module @210366b4
converter<1>        : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
message[2]          : Unable to make field private static final long java.util.concurrent.ConcurrentSkipListMap.serialVersionUID accessible: module java.base does not "opens java.util.concurrent" to unnamed module @210366b4
-------------------------------
	at com.thoughtworks.xstream.core.DefaultConverterLookup.lookupConverterForType(DefaultConverterLookup.java:88)
	at com.thoughtworks.xstream.XStream$1.lookupConverterForType(XStream.java:478)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:49)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:83)
	at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.marshallField(AbstractReflectionConverter.java:270)
	at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter$2.writeField(AbstractReflectionConverter.java:174)
	at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.doMarshal(AbstractReflectionConverter.java:262)
	at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.marshal(AbstractReflectionConverter.java:90)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:68)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:59)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:44)
	at com.thoughtworks.xstream.core.TreeMarshaller.start(TreeMarshaller.java:83)
	at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.marshal(AbstractTreeMarshallingStrategy.java:37)
	at com.thoughtworks.xstream.XStream.marshal(XStream.java:1266)
	at com.thoughtworks.xstream.XStream.marshal(XStream.java:1255)
	at com.thoughtworks.xstream.XStream.toXML(XStream.java:1228)
	at com.thoughtworks.xstream.XStream.toXML(XStream.java:1215)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.asXml(CpsThreadGroup.java:611)
	at org.jenkinsci.plugins.workflow.cps.CpsBodyExecutionTest.lambda$closureCapturesCpsBodyExecution$4(CpsBodyExecutionTest.java:241)
	at org.jvnet.hudson.test.RestartableJenkinsRule$3.evaluate(RestartableJenkinsRule.java:243)
	at org.jvnet.hudson.test.RestartableJenkinsRule$6.evaluate(RestartableJenkinsRule.java:291)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:606)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:833)

Evaluation

Of the core test suite and plugin BOM test suite, this is the only test failure I found with a java.util.concurrent reflection error. I think it would be desirable to eliminate java.util.concurrent from our Add-Opens directives if possible, since in most cases it is not necessary to serialize a concurrent data structure.

Solution

Interestingly enough, I found the tests passed by using a simple TreeMap wrapped in Collections.synchronizedNavigableMap. If the performance costs are acceptable (and I think they likely are), I think it is worth considering making this change. This would allow me to get rid of the java.util.concurrent Add-Opens directive in core's MANIFEST.MF in jenkinsci/jenkins#6392. Avoiding any Add-Opens directives at this early preview stage is much easier than getting rid of them later on.

Testing done

$ mvn clean verify -Djenkins.version=2.339 -Denforcer.skip -Djenkins-test-harness.version=1723.vcd938b_e66072 '-DargLine=--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED'

CCC @car-roll @dwnusbaum @jglick

…e in Pipeline: Groovy

jglick · 2022-03-22T13:50:19Z

Similar to #515: normally this is serialized using JBoss Marshalling to program.dat, which apparently uses some reflection system that does not trigger Java 17 restrictions (?), but for diagnostic purposes we can also use XStream which does.

jglick

Seems fine. This should not be a performance bottleneck.

Note that builds running prior to the upgrade will continue to use ConcurrentSkipListMap.

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java

basil · 2022-03-22T17:48:47Z

Testing done

With commit 57ac3de and Java 17, ran:

workflow-cps-plugin: mvn clean verify -Djenkins.version=2.339 -Denforcer.skip -Djenkins-test-harness.version=1723.vcd938b_e66072 '-DargLine=--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED'
pipeline-input-step-plugin: mvn clean verify -Dtest=org.jenkinsci.plugins.workflow.support.steps.input.InputStepTest#serialForm -Djenkins.version=2.339 -Denforcer.skip -Djenkins-test-harness.version=1723.vcd938b_e66072 '-DargLine=--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED'
workflow-basic-steps-plugin: mvn clean verify -Dtest=org.jenkinsci.plugins.workflow.steps.TimeoutStepTest#serialForm -Djenkins.version=2.339 -Denforcer.skip -Djenkins-test-harness.version=1723.vcd938b_e66072 '-DargLine=--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED'

basil · 2022-03-22T18:43:18Z

The Java 8/11 CI build is green as well.

jglick

Suggested a somewhat shorter and arguably more legible variant, but the behavior should be the same.

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

basil · 2022-03-22T19:58:12Z

It passed mvn verify on Java 17, so I added it to this PR as a commit. This PR could sure use some squashing (unless you're a Git archaeologist).

jglick

Short and sweet.

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

basil · 2022-03-23T05:07:57Z

Successful Java 17 BOM run with the incremental from this PR and no Add-Opens directives for java.util.concurrent. I believe this PR is ready for merge. As usual my preference is for a squash merge, but I have no strong preference and it is up to the maintainer.

basil · 2022-03-23T15:38:59Z

Looks like an agent was disconnected from this build. Any chance someone could re-run it so that we might be able to get a release of this?

jglick · 2022-03-23T15:41:48Z

Not much to be done until jenkins-infra/helpdesk#2849 is resolved.

basil · 2022-03-25T18:08:43Z

Better luck today?

jglick · 2022-03-25T22:01:29Z

https://ci.jenkins.io/job/Plugins/job/workflow-cps-plugin/job/master/403/console still broken it seems.

[JENKINS-68092] Serialization of java.util.concurrent data structur…

2512e58

…e in Pipeline: Groovy

basil mentioned this pull request Mar 22, 2022

Remove java.util.concurrent from --add-opens list jenkinsci/jenkins#6392

Merged

11 tasks

jglick added the bug label Mar 22, 2022

jglick requested review from dwnusbaum and car-roll March 22, 2022 13:46

jglick approved these changes Mar 22, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

dwnusbaum reviewed Mar 22, 2022

View reviewed changes

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java Outdated Show resolved Hide resolved

Deprecate threads and introduce two fresh fields.

57ac3de

jglick approved these changes Mar 22, 2022

View reviewed changes

basil and others added 2 commits March 22, 2022 12:27

Apply suggestions from code review

e37738c

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Fix compilation

f5a4b0b

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

Simplify

3b4d9c2

jglick approved these changes Mar 22, 2022

View reviewed changes

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java Outdated Show resolved Hide resolved

Apply suggestions from code review

8c3ff7f

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

dwnusbaum approved these changes Mar 22, 2022

View reviewed changes

Threads can never be null now

eb7d2e2

This comment was marked as resolved.

Sign in to view

jglick approved these changes Mar 22, 2022

View reviewed changes

jglick merged commit d0a8f6a into jenkinsci:master Mar 23, 2022

basil deleted the JENKINS-68092 branch March 23, 2022 15:37

basil restored the JENKINS-68092 branch March 26, 2022 04:46

jglick mentioned this pull request Mar 30, 2022

Use a concurrent map to manage timings #519

Merged

6 tasks

basil mentioned this pull request Sep 14, 2022

How much longer for XStream? x-stream/xstream#262

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-68092] Serialization of `java.util.concurrent` data structure in Pipeline: Groovy #518

[JENKINS-68092] Serialization of `java.util.concurrent` data structure in Pipeline: Groovy #518

basil commented Mar 22, 2022

jglick commented Mar 22, 2022

jglick left a comment

This comment was marked as resolved.

This comment was marked as outdated.

basil commented Mar 22, 2022

basil commented Mar 22, 2022

jglick left a comment

This comment was marked as resolved.

This comment was marked as outdated.

basil commented Mar 22, 2022

jglick left a comment

This comment was marked as resolved.

basil commented Mar 23, 2022

basil commented Mar 23, 2022

jglick commented Mar 23, 2022

basil commented Mar 25, 2022

jglick commented Mar 25, 2022

[JENKINS-68092] Serialization of java.util.concurrent data structure in Pipeline: Groovy #518

[JENKINS-68092] Serialization of java.util.concurrent data structure in Pipeline: Groovy #518

Conversation

basil commented Mar 22, 2022

Steps to reproduce

Expected results

Actual results

Evaluation

Solution

Testing done

jglick commented Mar 22, 2022

jglick left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

basil commented Mar 22, 2022

Testing done

basil commented Mar 22, 2022

jglick left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

basil commented Mar 22, 2022

jglick left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

basil commented Mar 23, 2022

basil commented Mar 23, 2022

jglick commented Mar 23, 2022

basil commented Mar 25, 2022

jglick commented Mar 25, 2022

[JENKINS-68092] Serialization of `java.util.concurrent` data structure in Pipeline: Groovy #518

[JENKINS-68092] Serialization of `java.util.concurrent` data structure in Pipeline: Groovy #518