Skip to content

WorkflowRun.reload must call super.onLoad #510

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 4 commits into from
Feb 7, 2025
Merged

WorkflowRun.reload must call super.onLoad #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc75582
`WorkflowRun.reload` must call `super.onLoad`
jglick Feb 7, 2025
a5fb661
In general we may need to call `super.onLoad` even if `!executionLoad…
jglick Feb 7, 2025
05ba748
Not right; now `onLoad` is called twice after loading from disk
jglick Feb 7, 2025
d63795c
I think I have it now
jglick Feb 7, 2025
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
23 changes: 19 additions & 4 deletions src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,13 @@ public String url() {
*/
volatile Boolean completed; // Non-private for testing

/**
* Whether {@link #onLoad} might be needed from {@link #reload}.
* Unfortunately {@link #reload} can be called either directly or implicitly via {@link #WorkflowRun(WorkflowJob, File)}
* and this is the only way to tell which of those cases this is.
*/
private transient volatile boolean loaded;
Copy link
Member

@dwnusbaum dwnusbaum Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, I find the naming of this field very confusing given the use down below. I guess the clearer thing would be to set the flag only from WorkflowRun(WorkflowJob, File), then change the name (to something like willCallOnLoadImplicitly) and invert the condition in reload here, but IIUC we can't do that because we have to call super(), which calls reload(), so we can't set a field in time.

I think "Early assignment to fields" in https://openjdk.org/jeps/492 would allow us to use a more straightforward approach here, right?

Copy link
Member Author

@jglick jglick Feb 7, 2025

Choose a reason for hiding this comment

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

the clearer thing would be to

I went through the same thought process, and yes I think JEP-492 would be helpful here.


/**
* Protects access to {@link #completed} etc.
* @see #getMetadataGuard
Expand Down Expand Up @@ -245,6 +252,7 @@ private BuildListener getListener() {
public WorkflowRun(WorkflowJob job) throws IOException {
super(job);
firstTime = true;
loaded = true;
checkouts = new PersistedList<>(this);
//System.err.printf("created %s @%h%n", this, this);
}
Expand Down Expand Up @@ -552,19 +560,26 @@ public boolean hasAllowKill() {
// super.reload() forces result to be FAILURE, so working around that
new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this);
synchronized (getMetadataGuard()) {
if (Boolean.TRUE.equals(completed) && executionLoaded) {
var _execution = execution;
if (_execution != null) {
_execution.onLoad(new Owner(this));
LOGGER.fine(() -> getExternalizableId() + " completed=" + completed + " executionLoaded=" + executionLoaded + " loaded=" + loaded);
if (Boolean.TRUE.equals(completed)) {
if (executionLoaded) {
var _execution = execution;
if (_execution != null) {
_execution.onLoad(new Owner(this));
}
}
}
if (loaded) {
super.onLoad();
} // else from WorkflowRun(WorkflowJob, File), and RunMap.retrieve will call onLoad
}
}

@Override protected void onLoad() {
super.onLoad();
try {
synchronized (getMetadataGuard()) {
loaded = true;
if (executionLoaded) {
LOGGER.log(Level.WARNING, "Double onLoad of build " + this, new Throwable());
return;
Expand Down
50 changes: 50 additions & 0 deletions src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,17 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.ExtensionList;
import hudson.model.Executor;
import hudson.model.InvisibleAction;
import hudson.model.Result;
import hudson.model.Run;
import hudson.model.TaskListener;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.logging.Level;
import jenkins.model.RunAction2;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.flow.FlowDurabilityHint;
import org.jenkinsci.plugins.workflow.flow.FlowExecution;
Expand All @@ -63,6 +69,7 @@
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.JenkinsSessionRule;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
Expand All @@ -71,6 +78,7 @@ public class WorkflowRunRestartTest {

@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
@Rule public JenkinsSessionRule story = new JenkinsSessionRule();
@Rule public LoggerRule logging = new LoggerRule();

@Issue("JENKINS-27299")
@Test public void disabled() throws Throwable {
Expand Down Expand Up @@ -437,4 +445,46 @@ public void onNewHead(FlowNode node) {
}
}
}

@Test public void reloadOwnerAndActions() throws Throwable {
logging.record(WorkflowRun.class, Level.FINE);
story.then(r -> {
var p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("", true));
var b = r.buildAndAssertSuccess(p);
var a = new A();
b.addAction(a);
b.save();
assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner()));
assertThat("attached once", a.attached, is(1));
assertThat("not yet loaded", a.loaded, is(0));
b.reload();
assertThat("right owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner()));
a = b.getAction(A.class);
assertThat("not attached in this instance", a.attached, is(0));
assertThat("loaded", a.loaded, is(1));
});
story.then(r -> {
var p = r.jenkins.getItemByFullName("p", WorkflowJob.class);
var b = p.getBuildByNumber(1);
var a = b.getAction(A.class);
assertThat("after restart, not attached in this instance", a.attached, is(0));
assertThat("after restart, loaded", a.loaded, is(1));
b.reload();
assertThat("after restart, owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner()));
a = b.getAction(A.class);
assertThat("after restart, not attached in this instance", a.attached, is(0));
assertThat("after restart, loaded", a.loaded, is(1));
});
}
private static final class A extends InvisibleAction implements RunAction2 {
transient volatile int attached, loaded;
@Override public void onAttached(Run<?, ?> r) {
attached++;
}
@Override public void onLoad(Run<?, ?> r) {
loaded++;
}
}

}
10 changes: 0 additions & 10 deletions src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -620,15 +619,6 @@ private static String checkoutString(GitSampleRepoRule repo, boolean changelog,
", userRemoteConfigs: [[url: $/" + repo.fileUrl() + "/$]]])\n";
}

@Test public void reloadOwner() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "reloadOwner");
p.setDefinition(new CpsFlowDefinition("", true));
WorkflowRun b = r.buildAndAssertSuccess(p);
assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner()));
b.reload();
assertThat("right owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner()));
}

// This test is to ensure that the shortDescription on the CancelCause is escaped properly on summary.jelly
@Issue("SECURITY-3042")
@Test public void escapedDisplayNameAfterAbort() throws Exception {
Expand Down