-
Notifications
You must be signed in to change notification settings - Fork 194
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-45327] Pickle EnvActionImpl to prevent multiple instances from existing after resumption #539
Conversation
…om existing after resumption
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.DoNotUse; | ||
import org.kohsuke.stapler.export.Exported; | ||
import org.kohsuke.stapler.export.ExportedBean; | ||
|
||
@ExportedBean | ||
public class EnvActionImpl extends GroovyObjectSupport implements EnvironmentAction.IncludingOverrides, Serializable, RunAction2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause Pipelines which already have a serialized instance of EnvActionImpl
to fail, but those instances would have thrown an NPE anyway if they were used, so my thinking is that this should not affect a significant number of users. We could leave it in place if we want to be extra careful though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing Serializable
makes sense here.
src/main/java/org/jenkinsci/plugins/workflow/cps/EnvActionImpl.java
Outdated
Show resolved
Hide resolved
* Prevents multiple instances of {@link EnvActionImpl} from existing for a single Pipeline after a Jenkins restart | ||
* in case {@code env} is serialized into the program. | ||
*/ | ||
private static class EnvActionImplPickle extends Pickle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no internal state, we just use it as a hook to return EnvActionImpl.forRun(owner.getExecutable())
during deserialization, completely discarding the serialized object.
src/main/java/org/jenkinsci/plugins/workflow/cps/EnvActionImpl.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java
Outdated
Show resolved
Hide resolved
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.DoNotUse; | ||
import org.kohsuke.stapler.export.Exported; | ||
import org.kohsuke.stapler.export.ExportedBean; | ||
|
||
@ExportedBean | ||
public class EnvActionImpl extends GroovyObjectSupport implements EnvironmentAction.IncludingOverrides, Serializable, RunAction2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing Serializable
makes sense here.
See JENKINS-45327.
Right now, if you use
env
in a Pipeline in a way that causesEnvActionImpl
to be serialized into the program's state, then after resumption you end up with a distinct instance ofEnvActionImpl
that is unrelated to the action attached to theWorkflowRun
, so theowner
for the deserializedEnvActionImpl
is null, andgetProperty
andsetProperty
throw NPEs.This PR fixes the issue by pickling
EnvActionImpl
so that it can be rehydrated using the action attached to theWorkflowRun
, which maintains the expected singleton semantics and prevents issues.