On Fri, May 22, 2026 at 12:46 PM Mark Thomas <[email protected]> wrote: > > 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.
Very good refactoring. I think the process is normal: - This is async so ... We still do a "normal" impl. - People use the impl, find real issues along with a lot of misuse. - We start adding special handling for the common misuse paths. It looks fishy of course, but it works. - Code review says it looks fishy. <- New step ! - Come up with a more general and robust pattern in a refactoring. - Code review again. <- We can now do an additional review of large extensive refactorings, making them a bit safer So overall it seems this adds value, for now at least. > 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). After a long while (async = complicated), it came up considering some items but it looks ok (mostly low what ifs, most from the internal methods that are actually ok). Rémy > Mark > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
