Static loggers

[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.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Static loggers

[Mark Thomas <ma...@apache.org>]

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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Static loggers

[Konstantin Kolinko <kn...@gmail.com>]

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.

Reviewing r1832692 : OK

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.

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.


Alternatives:
a) a helper method that unsets TCCL, creates a logger and restores TCCL.

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

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org