Good catch, I recall a similar bug using fully asynchronous logging when the
exception handler threw an error:
https://issues.apache.org/jira/browse/LOG4J2-2333
While it's generally not considered good practice to "handle" errors, in this
case it's far better than not logging! I've seen this manifest a number of
times from plugins built against old jars throwing linker errors. The proposed
change sounds great, thanks!
-ck
On Wed, Oct 28, 2020, at 16:05, Volkan Yazıcı wrote:
> Hello,
>
> While logging, we sometimes notice that the entire logging infra comes to a
> halt, even though the rest of the application still works perfectly fine. I
> have figured, appenders wrapped by AsyncAppender can throw a
> java.lang.Throwable that is not a subclass of java.lang.Exception and such
> an exception kills the AsyncAppender worker thread. Consider the following
> snippet from AsyncAppender.java in 2.x branch:
>
> boolean callAppenders(final LogEvent event) {
> boolean success = false;
> for (final AppenderControl control : appenders) {
> try {
> control.callAppender(event);
> success = true;
> } catch (final Exception ex) {
> // If no appender is successful the error appender will get it.
> }
> }
> return success;
> }
>
> Further, this is the relevant AppenderControl#tryCallAppender(LogEvent)
> method:
>
> private void tryCallAppender(final LogEvent event) {
> try {
> appender.append(event);
> } catch (final RuntimeException ex) {
> handleAppenderError(event, ex);
> } catch (final Exception ex) {
> handleAppenderError(event, new AppenderLoggingException(ex));
> }
> }
>
> To avoid AsyncAppender.AsyncThread getting killed, I propose, in
> tryCallAppender(), replacing the java.lang.Exception catch clause with
> java.lang.Throwable instead. Objections? (If there are none, I will push
> this to both master and release-2.x branches with some unit tests.)
>
> To get some inspiration, I have checked the
> java.util.concurrent.ThreadPoolExecutor#runWorker() method:
>
> try {
> task.run();
> } catch (RuntimeException x) {
> thrown = x; throw x;
> } catch (Error x) {
> thrown = x; throw x;
> } catch (Throwable x) {
> thrown = x; throw new Error(x);
> }
>
> This is inline with the change I propose for tryCallAppender().
>
> For the records, the most frequent Throwable we encounter that is a super
> class of Exception is ExceptionInInitializerError, in case you are
> interested in.
>
> Kind regards.
>
