On 01/06/18 16:35, Konstantin Kolinko wrote: > 2018-06-01 17:13 GMT+03:00 Mark Thomas <ma...@apache.org>: >> Hi, >> >> I've stumbled across a problem with some static loggers. The general >> pattern is: >> >> - TCCL is web application class loader >> - static logger is created >> - static logger is associated with web application class loader >> - web application is undeployed >> - appenders are removed from logger >> - further logger output is lost >> >> In most cases we avoid this issue by using a non-static logger. I've >> found a few places where we need to switch from static to non-static >> loggers. That in turn is triggering some refactoring (as static methods >> can't access a non-static logger). >> >> I should have something ready to commit later today. > > So effectively now each web application can have its own logging > configuration (in WEB-INF/classes/logging.properties) for those > classes.
Yes. Although before this was also possible and it would end up being controlled by whichever web application triggered the logger's creation. > Reviewing r1832692 : OK Thanks for looking at this. > 1. CallbackHandlerImpl: > > It exists in one instance only.See "private static CallbackHandler > instance;", and the constructor of this class is private. > > If a web application creates this class, you will have the same > problem with lifespan of the logger when the application is > undeployed. The bug is not fixed here. I didn't see this one in my testing. However, that testing wasn't exhaustive. I'll take a look at this one. > 2. Jasper's SecurityClassLoad class > It is a bit odd that its logging can be controlled by a web > application, but it is OK. > > 3. Maybe add a comment, e.g. // Non-static. A web application can have > its own configuration of logging. > Not really needed, but my first thought was that I want to see one. Fair point. The ones we have fixed previously have: // must not be static I'll look at adding those everywhere there isn't one. > Alternatives: > a) a helper method that unsets TCCL, creates a logger and restores TCCL. I thought about that. My concern was that you only want to unset the TCCL for Tomcat provided classes. That starts to look like a maintenance headache. You also need to deal with running under a security manager and we'd need to check performance. Overall, there were sufficient (potential) issues I went for a different option. > b) preload the class, > but a system property reading bug a month ago showed that simple > preloading as done by SecurityClassLoad classes does not cause > initialization of static fields. > https://bz.apache.org/bugzilla/show_bug.cgi?id=62350#c7 Indeed. We'd need to init the class as well. Not a big deal. I opted for making the loggers non-static as that was consistent with what we had done previously. The pre-loading has been more for security manager issues. Thanks again for the review. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
