[JENKINS-49601] Fix NPE in ReplayCause when replaying Runs. fixes #1401#1729
[JENKINS-49601] Fix NPE in ReplayCause when replaying Runs. fixes #1401#1729rbrindl wants to merge 4 commits intojenkinsci:masterfrom
Conversation
rbrindl
changed the title
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
The PR does not break anything exactly, but it is not really solving the problem, whatever that is. Is onAddedTo / onLoad not getting called for some reason? Or getting called, but after some other code is using the cause? The tests here are not helpful since they do not correspond to a real user scenario reproducing the NPE.
| // construct cause, don't call onAddedTo yet. | ||
| ReplayCause cause = new ReplayCause(original); | ||
| assertNull(cause.getRun()); | ||
|
|
||
| // verify print() does not throw NPE. | ||
| TaskListener listener = StreamTaskListener.fromStdout(); | ||
| cause.print(listener); |
There was a problem hiding this comment.
This is meaningless; it does not correspond to a real scenario. A cause should not be used until it is attached to something.
There was a problem hiding this comment.
I would not say its meaningless, because this is exactly what happens on our Jenkins instances about every two weeks when someone replays a job. So it IS a real scenario: there is a ReplayCause with a run that is null, and that's exactly what these tests check. I don't know (or can only speculate) why this is.
But since the constructor of ReplayCause does not set run, but only onLoad() and onAddedTo() do, there definitely is a window of opportunity in which run is null.
The same goes for the de-serialization case.
I didn't want to make this more complicated than necessary: There is a member that may be null when dereferenced, so a null-check seems to be in order. I can drop the tests, if you like.
| // serialize and deserialize the cause, which should clear the transient run field. | ||
| XStream2 xs = new XStream2(); | ||
| String xml = xs.toXML(cause); | ||
|
|
||
| Object obj = xs.fromXML(xml); | ||
| if (obj instanceof ReplayCause) { | ||
| ReplayCause deserialized = (ReplayCause) obj; | ||
| assertNull(deserialized.getRun()); |
2 participants
Fixes #1401. [edited by jglick to follow normal linking style]
The fix checks the member variable
runandrun.getParent()for null and returns null in that caseOtherwise it calls
getBuildNumber()on the parent, as in the original code.Testing done
Two unit tests that demonstrate both failure modes
Submitter checklist