Issue #4351 Lazy servlet init is not atomic

[janbartel]

Issue #4351

The code changes in commit d48c258#diff-0d419a5cace2d3e164cd481d96f24734 made the lazy initialization of a servlet non-atomic. Specifically, the bug report shows the situation where request1 comes into the servlet, creates the instance and then calls servlet.init(); meanwhile request2 comes into the servlet and checks if the servlet is non-null, then proceeds to use it. The code around the creation/testing of the servlet instance and the call to servlet.init() needs to be atomic so that request2 cannot use the instance until request1 has finished the call to servlet.init().

The test case I wrote is based on the repro provided by the OP. I fear that as it is reliant on some sleeps to provoke the correct timing to cause the situation, it is likely to be flakey. Any suggestions for a better test are welcome.

[gregw]

LGTM

[sbordet]

Since we are modifying ServletHolder, there is an evident search/replace mistake at line 629.
It must be restored to org.apache.catalina.jsp_classpath.

The fix looks good.

The test needs a overhaul because it's way too complicated - it really only need to test the value of the counter, but there are asserts in 3 classes and it's difficult to follow what it does (and lasts to much time).

[joakime]

Am I right in assuming that this only applies to Servlets that do not have the init-order or load-on-startup configurations set?

So in a hypothetical setup where let say a Filter.init() adds a ServletContext attribute, and a Servlet that needs that attribute, would this PR solve for that as well? (or is this scenario not impacted?)
Taking into account the example reproduction case laid out in #4351

[janbartel]

@joakime I don't believe that such a scenario is involved in this fix at all. Filters are never lazily initialized, they are always initialized when their context starts up. This concerns purely the lazy initialization of servlets.

[sbordet]

@janbartel I could not stand empty new lines and indents at 2 spaces rather than 4, so I went on and cleaned up the test code.