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.
If this is set, use it instead of ServletRequest.getServerPort() for purposes of getRootUrlFromRequest().
Also treat the default port as scheme-specific in that method (which presumes that we in fact got the reported port right).
And enhance the reverse proxy setup monitor to validate that the Referer header (/manage)
actually matches what we have computed from getRootUrlFromRequest;
if it does not, something is messed up, though it may require some digging to find what.
(Would be better to let the monitor specify the exact problem it determined,
though this is not always actually possible;
for example if you are missing AllowEncodedSlashes NoDecode in Apache,
you just get a 404 from Apache without even getting to Jenkins.)
The basic fix is to use ${rootURL} plus full model object URLs rather than relying on ${jobBaseUrl}.
This is made trickier by the fact that the model object URLs are computed inside ProgressiveRendering.compute,
and therefore will not be correct when nondefault views are in the crumb trail unless the original request information is present.
So modifying ProgressiveRendering to preserve a copy of the original request for use during computation.
(This could probably be used to simplify parts of AsynchPeople as well.)
Also improving AbstractItem.getUrl to properly construct a URL including views even when the current page is not inside the item;
it should suffice for some ancestor of the current item (or a view thereof) to be in the ancestor list of this page.
Jesse Glick