Simplified PlaceholderTask
#354
Conversation
| // Thus we're guaranteeing the execution began and the Node is now unknown. | ||
| // Theoretically it's safe to simply fail earlier when rehydrating any EphemeralNode... but we're being extra safe. | ||
| if (placeholder.getCookie() != null && Jenkins.get().getNode(placeholder.getAssignedLabel().getName()) == null ) { | ||
| if (placeholder.hasStarted() && Jenkins.get().getNode(placeholder.getAssignedLabel().getName()) == null ) { |
There was a problem hiding this comment.
(just better encapsulation)
| throw new IllegalStateException("queue refused " + task); | ||
| } | ||
| // Try to avoid having distinct a instance of PlaceholderTask here compared to any previously-scheduled task. | ||
| task = (ExecutorStepExecution.PlaceholderTask)item.task; |
There was a problem hiding this comment.
Reverting this bit of #344 which should now be unnecessary: any instances in memory ought to be interchangeable at this point.
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Show resolved
Hide resolved
|
|
||
| @Override public String toString() { | ||
| return "ExecutorStepExecution.PlaceholderTask{runId=" + runId + ",label=" + label + ",context=" + context + ",cookie=" + cookie + ",auth=" + auth + '}'; | ||
| return "ExecutorStepExecution.PlaceholderTask{label=" + label + ",context=" + context + '}'; |
There was a problem hiding this comment.
Reducing noise and redundancy. CpsStepContext.toString already encodes the Run#externalizableId; and cookie is now rarely interesting. auth was never interesting unless you installed authorized-project, now deprecated.
| if (cookie == null) { | ||
| private static void finish(StepContext context, @CheckForNull final String cookie) { | ||
| RunningTask runningTask = RunningTasks.remove(context); | ||
| final AsynchronousExecution execution = runningTask.execution; |
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Outdated
Show resolved
Hide resolved
| return "PlaceholderExecutable:" + PlaceholderTask.this; | ||
| } | ||
|
|
||
| private static final long serialVersionUID = 1L; |
There was a problem hiding this comment.
Unclear why this was here, when the PlaceholderExecutable is not Serializable.
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
… task completed), and `RunningTasks` add/remove timing was off
|
If d1a2aa2 fixes test failures in PCT, would it make sense to add a test (or multiple tests) here for whatever it is fixing? |
|
Probably yes. The clear problem was the While working on that I noticed that |
| import org.jvnet.hudson.test.TestExtension; | ||
|
|
||
| /** | ||
| * Like {@link ExecutorStepTest} but not using {@link Parameterized} which appears incompatible with {@link TestExtension}. |
There was a problem hiding this comment.
Did not notice in #323 that PipelineOnlyTaskDispatcher was not actually getting loaded. (queueTaskOwnerCorrectWhenRestarting passed with or without it.)
| var p = r.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition("node('test') {}", true)); | ||
| r.assertLogContains("Running on test-1", r.buildAndAssertSuccess(p)); | ||
| r.assertLogContains("Running on test-2", r.buildAndAssertSuccess(p)); |
There was a problem hiding this comment.
Without d1a2aa2, fails
java.lang.AssertionError:
Expected: a string containing "Running on test-2"
but: was "Started
[Pipeline] Start of Pipeline
[Pipeline] node
Running on test-1 in …/agents/test-1/workspace/p
[Pipeline] {
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Finished: SUCCESS
"
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at org.jvnet.hudson.test.JenkinsRule.assertLogContains(JenkinsRule.java:1518)
at org.jenkinsci.plugins.workflow.support.steps.ExecutorStep2Test.lambda$cloud$2(ExecutorStep2Test.java:121)
2 participants
Working on the same test as in #353, I found a (locally unreproducible) case where it appears that
Queue.cancelfromPlaceholderTask.Callback.finishedtriggeredCancelledItemListener.PlaceholderTask.stoppingwas designed to prevent exactly this; the fact that it did not suggests that there are multiplePlaceholderTasks involved. As in #344, the root design flaw is that there is mutable state inPlaceholderTaskyet it is difficult to ensure Java object identity here. So here I am moving thestoppingfield to transient global state (RunningTasks). I am also restoringcookieto its original purpose of an actual environment variable value used for nothing more than killing off stray background processes at the end of the block; and usingStepContextas the primary key whenever we need to look up state. I did consider makingPlaceholderTaskactually immutable but this would require changing the storage of thelabelandcookiefields, which is more difficult for two reasons: we need to actually read these values from builds running before the upgrade; and we would need to find some other spot to persist them (such as fields inExecutorStepExecutionperhaps). I think these fields are not real problems anyway, since they are only mutated once, inPlaceholderExecutable.run, and before that point there does not seem to be a risk of there being multiple copies of thePlaceholderTask: the known problems involve deserialization from both the queue and fromExecutorStepDynamicContext.task, butExecutorStepDynamicContextis not created until aftercookieandlabelhave been written.