On 21/05/2026 12:36, Mark Thomas wrote:
On 21/05/2026 11:55, Rémy Maucherat wrote:
On Thu, May 21, 2026 at 12:50 PM Mark Thomas <[email protected]> wrote:
On 20/05/2026 14:44, [email protected] wrote:
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new f0a584792e Generalize null checks in AsyncContext
f0a584792e is described below
commit f0a584792e32da7d18708de13f0652cf84dded65
Author: remm <[email protected]>
AuthorDate: Wed May 20 15:44:17 2026 +0200
Generalize null checks in AsyncContext
Is this the right approach?
In most (all?) of these cases if context is null, the method should not
have been called in the first place. Wouldn't throwing
IllegalStateException be better here. That would be consistent with
getRequest() and getResponse().
Unsure, we started adding some in the paths that were commonly
reported by users, so it simply generalizes this. If we add ISE, then
it means lots of logging and people may complain.
The previous null checks look to be handling race conditions during
shutdown.
If users were seeing NPEs that are addressed by the new checks, they'd
be seeing a lot of logging anyway for the NPEs.
I'm going to remind myself how the shutdown race conditions happen, make
sure all the possible NPEs from those don't cause problems and then
throw ISEs for the others.
I'm not sure at this point how many of the code paths are impact by the
race condition.
It took far longer than I thought it would to get my head around this.
As a result, I have added a few comments to hopefully make it easier
next time.
I ended up taking a different approach than the one I originally envisaged.
The concurrency risk comes from applications retaining references to the
AsyncContext in non-container threads and trying to use them beyond the
point they are valid. This can happen at shutdown but that is an
instance of the more general case of the async request timing out,
Tomcat cleaning it up but the application continuing to process on a
non-container thread. Other application bugs can have similar results.
I therefore took the following approach:
- AsyncContext methods that can be called by an application take local
copies of any instance variables used, validate the instance is in a
valid state and then execute the method.
- AsyncContext methods that can only be called by Tomcat internal
methods don't check state on the basis that Tomcat manages state and
only calls those methods when the state is valid.
- To avoid concurrency issues with the state of the local variables, a
dedicated atomic flag is introduced to track whether recycled has been
called and create a "happens-before" relationship with the local
copies.
To add to the fun, AI analysis may well report race conditions around
recycle but they are false positives (which the AI will figure out if
pressed).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
