Skip to content

Making sure ArgumentsAction is attached in time for GraphListener.Synchronous notification #254

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.

Sign up for GitHub

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
jglick merged 1 commit into from
Oct 4, 2018
Merged

Making sure ArgumentsAction is attached in time for GraphListener.Synchronous notification #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@
<java.level>8</java.level>
<no-test-jar>false</no-test-jar>
<git-plugin.version>3.1.0</git-plugin.version>
<workflow-support-plugin.version>2.17</workflow-support-plugin.version>
<scm-api-plugin.version>2.0.8</scm-api-plugin.version>
<workflow-support-plugin.version>2.21-rc578.95f4df4d2c32</workflow-support-plugin.version> <!-- TODO https://github.com/jenkinsci/workflow-support-plugin/pull/75 -->
<scm-api-plugin.version>2.2.6</scm-api-plugin.version>
<groovy-cps.version>1.24</groovy-cps.version>
<structs-plugin.version>1.15</structs-plugin.version>
</properties>
Expand Down
42 changes: 21 additions & 21 deletions src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@
import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
import static org.jenkinsci.plugins.workflow.cps.ThreadTaskResult.*;

import org.jenkinsci.plugins.workflow.actions.TimingAction;
import org.jenkinsci.plugins.workflow.cps.actions.ArgumentsActionImpl;
import org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode;
import org.jenkinsci.plugins.workflow.cps.nodes.StepEndNode;
Expand All @@ -78,7 +77,6 @@
import org.jenkinsci.plugins.workflow.cps.steps.LoadStep;
import org.jenkinsci.plugins.workflow.cps.steps.ParallelStep;
import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
import org.jenkinsci.plugins.workflow.graph.AtomNode;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
import org.jenkinsci.plugins.workflow.steps.MissingContextVariableException;
Expand Down Expand Up @@ -229,14 +227,31 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) {

if (ps.body == null && !hack) {
an = new StepAtomNode(exec, d, thread.head.get());
// TODO: use CPS call stack to obtain the current call site source location. See JENKINS-23013
thread.head.setNewHead(an);
} else {
an = new StepStartNode(exec, d, thread.head.get());
thread.head.setNewHead(an);
}

final CpsStepContext context = new CpsStepContext(d,thread,handle,an,ps.body);
// Ensure ArgumentsAction is attached before we notify even synchronous listeners:
final CpsStepContext context = new CpsStepContext(d, thread, handle, an, ps.body);
try {
Copy link
Member

@svanoort svanoort Oct 4, 2018

Choose a reason for hiding this comment

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

I think that you want the classloader switch to happen before the try block for safety, no? Or is there a reason why we shouldn't continue running this logic under the CpsVmExecutorService.ORIGINAL_CONTEXT_CLASS_LOADER.get() anymore?

Copy link
Member Author

@jglick jglick Oct 4, 2018

Choose a reason for hiding this comment

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

It never needed to be run with a special CCL to begin with. That is just where you happened to drop that new code. The CCL is for user code, mainly StepExecution.start implementations.

Copy link
Member

@svanoort svanoort Oct 4, 2018

Choose a reason for hiding this comment

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

The ORIGINAL_CONTEXT_CLASSLOADER is for non-Groovy code though IIRC -- and IIRC we are switching away from the GroovyClassLoader to avoid stuff being loaded there (and potential memory leaks or issues with classloaders not matching).

I think you still want this to be happening with the non-Groovy classloader.

Copy link
Member Author

@jglick jglick Oct 4, 2018

Choose a reason for hiding this comment

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

The ORIGINAL_CONTEXT_CLASSLOADER is for non-Groovy code though

For non-Groovy plugin code, yes.

we are switching away from the GroovyClassLoader to avoid stuff being loaded there (and potential memory leaks or issues with classloaders not matching)

We are switching to the CCL used for general plugin Java code while we run the general plugin Java code: StepDescriptor.newInstance, Step.start, and (especially) StepExecution.start. ArgumentsActionImpl has nothing to do with this.

Copy link
Member Author

@jglick jglick Oct 4, 2018

Choose a reason for hiding this comment

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

FTR this system is from #50. #98 happened to drop ArgumentsActionImpl into the same try block as a convenience AFAICT—because it was checking for instanceof ParallelStep, which can only be done after instantiating the Step. This PR instead checks the StepDescriptor, which can be done earlier.

// No point storing empty arguments, and ParallelStep is a special case where we can't store its closure arguments
if (ps.namedArgs != null && !(ps.namedArgs.isEmpty()) && isKeepStepArguments() && !(d instanceof ParallelStep.DescriptorImpl)) {
// Get the environment variables to find ones that might be credentials bindings
Computer comp = context.get(Computer.class);
EnvVars allEnv = new EnvVars(context.get(EnvVars.class));
if (comp != null && allEnv != null) {
allEnv.entrySet().removeAll(comp.getEnvironment().entrySet());
}
an.addAction(new ArgumentsActionImpl(ps.namedArgs, allEnv));
}
} catch (Exception e) {
// Avoid breaking execution because we can't store some sort of crazy Step argument
LOGGER.log(Level.WARNING, "Error storing the arguments for step: " + d.getFunctionName(), e);
}

// TODO: use CPS call stack to obtain the current call site source location. See JENKINS-23013
thread.head.setNewHead(an);

Step s;
boolean sync;
ClassLoader originalLoader = Thread.currentThread().getContextClassLoader();
Expand All @@ -247,21 +262,6 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) {
d.checkContextAvailability(context);
Thread.currentThread().setContextClassLoader(CpsVmExecutorService.ORIGINAL_CONTEXT_CLASS_LOADER.get());
s = d.newInstance(ps.namedArgs);
try {
// No point storing empty arguments, and ParallelStep is a special case where we can't store its closure arguments
if (ps.namedArgs != null && !(ps.namedArgs.isEmpty()) && isKeepStepArguments() && !(s instanceof ParallelStep)) {
// Get the environment variables to find ones that might be credentials bindings
Computer comp = context.get(Computer.class);
EnvVars allEnv = new EnvVars(context.get(EnvVars.class));
if (comp != null && allEnv != null) {
allEnv.entrySet().removeAll(comp.getEnvironment().entrySet());
}
an.addAction(new ArgumentsActionImpl(ps.namedArgs, allEnv));
}
} catch (Exception e) {
// Avoid breaking execution because we can't store some sort of crazy Step argument
LOGGER.log(Level.WARNING, "Error storing the arguments for step: "+d.getFunctionName(), e);
}

// Persist the node - block start and end nodes do their own persistence.
CpsFlowExecution.maybeAutoPersistNode(an);
Expand Down