Adapted from #1375 by @daniel-beck: omitting change to User construction, and adding test.
(cherry picked from commit 088edabb31)
Conflicts:
changelog.html
For reasons TBD, sometimes CI tests fail with (1.565.x):
java.lang.InterruptedException: null
at java.lang.Object.wait(Native Method)
at java.lang.Object.wait(Object.java:503)
at org.jvnet.hudson.reactor.Reactor.execute(Reactor.java:267)
at jenkins.InitReactorRunner.run(InitReactorRunner.java:44)
at jenkins.model.Jenkins.executeReactor(Jenkins.java:915)
at jenkins.model.Jenkins.<init>(Jenkins.java:814)
at hudson.model.Hudson.<init>(Hudson.java:82)
at org.jvnet.hudson.test.JenkinsRule.newHudson(JenkinsRule.java:539)
at org.jvnet.hudson.test.JenkinsRule.before(JenkinsRule.java:331)
at org.jvnet.hudson.test.JenkinsRule$2.evaluate(JenkinsRule.java:479)
Previously, basic auth header was processed from two different servlet
filters in a single filter chain.
In case the 1st filter (ApiTokenFilter) manages to authenticate the
request, the 2nd filter (BasicProcessingFilter) tries to avoid
interpreting the API token as the password and failing authentication
(see BasicProcessingFilter.authenticationIsRequired), but the check
feels rather fragile.
Although I did eventually discover that the original problem (ZD-19640)
was not caused by this, I've already implemented & tested this change,
and this feels like a good work to be wasted, so I'm pushing this in
anyway.
Refrence: ZD-19640
Correcting change made in #1176, which introduced an NPE, to restore original logic merely wrapped in a synchronized block.
Reproduced NPE in new functional test (original bug probably very hard to reproduce).
Confirmed that dropLinksAfterGC and dropLinksAfterGC2 both fail in the expected way (b1a.nextBuild == b2) after commenting out the call to createReference().clear() in dropLinks.
(Also that they fail as expected in assertNotSame if the reference is not cleared at all.)
As I step-executed the code, I discovered b2.getPreviousBuild() was getting invoked
between BRHF.drop(b1) and b2.dropLinks() call, in PeepholePermalink.RunListenerImpl.onDeleted()
because Run.delete() calls RunListener.fireDeleted(this).
Thus in effect the sequence of the call order was as follows:
assertEquals(1, BRHF.drop(b1));
b2.getPreviousBuild(); // happens indirectly in PeepholePermalink
b2.delete();
FreeStyleBuild b1a = b2.getPreviousBuild();
This defeats the purpose of the fix, because in this call sequence, by b2.dropLinks()
as implemented before f1430a2 will correctly fix up b1a.nextBuildR to b3.
For the test to work as intended, it is important that b2.previousBuildR.get()==null
during dropLinks(). That is what causes b2.getPreviousBuild() in the next line to go
load b1a, and sets up b1a.nextBuildR to b2, and trigger the assertion error.
Added a code to remove all RunListeners. With this change, the test now fails without
the fix in f1430a2.
I'm not sure if I understand how the original fix in PR #1190 (f1430a2) addresses the problem.
My understanding is of the problem is as follows:
- someone deletes 'b2' but holds on to its reference (could be different threads)
- someone calls b2.getPreviousBuild()
- if b2.previousBuildR is null, then this triggers the loading of b1, and
that process sets up a bi-di link via b1<->b2 via b1.nextBuildR <-> b2.previousBuildR
- this makes b1.getNextBuild() incorrectly return b2
Presumably f1430a2 addresses this somehow, but I think I can induce this situation in other ways,
which is what dropLinksAfterGC2() does.
In this test,
initial state:
b1 <-> b2 <-> b3
we load everyone and connect them all up
after deleting b2:
b1 <--------> b3
^ ^
+---- b2 ----+
b1 and b3 points each other, and b2 still refers to each side
after dropping b1:
b2 --> b3
now, here, when I do b2.getPreviousBuild(), it loads b1a and it sets b1a.nextBuildR to b2.
b1a <-> b2 --> b3
So I claim this is a proof that the fix is incomplete, even for the problem JENKINS-22395 has discovered.
I don't think that the problem is for the dropLinks call to fail to patch up references.
The problem is that b2.getPreviousBuild() forcing b1 to point to b2, because if b2 is deleted and assumed to be
invalid, then no matter what bX this method will find you never want bX.nextBuildR to point to b2.
Kohsuke Kawaguchi