Re: Catching Throwable's in AsyncAppender worker

[Volkan Yazıcı <vo...@gmail.com>]

I have pushed 56436a to release-2.x. Would you mind taking a look at it,
please? If there are no objections, I want to port it to master as well.

On Wed, Oct 28, 2020 at 9:05 PM Volkan Yazıcı <vo...@gmail.com>
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.
>

Re: Catching Throwable's in AsyncAppender worker

[Matt Sicker <bo...@gmail.com>]

Amusingly enough, we used to have almost the same exact sneaky throws
utility method in Throwables. I guess that was removed at some point.

On Thu, Nov 5, 2020 at 09:24 Volkan Yazıcı <vo...@gmail.com> wrote:

> I have pushed 56436a to release-2.x. Would you mind taking a look at it,
> please? If there are no objections, I want to port it to master as well.
>
> On Wed, Oct 28, 2020 at 9:05 PM Volkan Yazıcı <vo...@gmail.com>
> 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.
> >
>