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

[jglick]

Downstream of jenkinsci/workflow-support-plugin#75. Extracted from #252 as an independently releasable fix necessary to avoid regressing StepNodeTest (specifically, proper display of metasteps in the classic UI console) after changes in JEP-210. Specifically, as of jenkinsci/workflow-job-plugin#27 (comment) FlowNode.getDisplayFunctionName is called from within a synchronous listener. (Formerly this was an asynchronous listener, which sounds wrong but then again copyLogs was asynchronous too so who cared?) And this method call pays attention to ArgumentsAction to display, for example, archiveArtifacts rather than step (the generic function name of CoreStep). So we need to ensure that at least this basic metadata about the FlowNode is actually present by the time listeners are told about it. Some other metadata gets added later, like LabelAction, but we tolerate that—the new console UI renders the label via a ConsoleNote, potentially much later, at which point the label is known.

[svanoort]

For those who will be equally confused about the key missing piece: this depends on the upstream PR because we need to be able to get the Environment a bit early (before we have the FlowNode set up and everything).

[jglick]

Note that the entire reason ArgumentsActionImpl needs an EnvVars to begin with is a hack. See JENKINS-47101 for discussion.

[svanoort]

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?

[jglick]

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.

[svanoort]

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.

[jglick]

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.

[jglick]

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.

[svanoort]

The contextClassLoader change makes me twitchy, but I can't see an obvious bug.