WorkflowRun.reload must call super.onLoad
#510
Conversation
| 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)); |
There was a problem hiding this comment.
Without patch, this is zero, as in observed stack traces.
dwnusbaum
left a comment
dwnusbaum
left a comment
There was a problem hiding this comment.
Seems fine if it works and passes PCT
| * 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
the clearer thing would be to
I went through the same thought process, and yes I think JEP-492 would be helpful here.
2 participants
Amending #472 as I think I finally tracked down the problem with
TestResultActionthat I was trying without success to fix in #485. Ultimately this feels like a bug, or at least an anomaly, in core since jenkinsci/jenkins@50f7aa3 defined theRun.reloadmethod with a vague comment saying thatonLoadwas not to be called sincereloadwould be used from oneRunconstructor overload (which it is; in this PR that call site will not activate the patched behavior) but also that some unidentified uses do not want to be called multiple times. The latter part makes no sense, since this is on a freshRunAction2instance each time, and due to lazy loading that might already be true. This only seems to affect CloudBees CI, which calls this method in HA mode, since I am not aware of any call toRun.reloadexcept via the load-from-disk constructor.