[GitHub] [logging-log4j2] vy commented on pull request #452: LOG4J2-2972 Respawn AsyncAppender thread on failures.

[GitBox <gi...@apache.org>]

vy commented on pull request #452:
URL: https://github.com/apache/logging-log4j2/pull/452#issuecomment-752224064


   > I think the only way this can happen is if someone calls [Thread.stop()](https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#stop--), so we must consider the intent when that occurs. Is it accidental, where the asyncappender interaction is collateral damage, or is the system attempting to stop all threads? In the former case we may want to avoid breaking the logger, but in the latter we would not want to restart the background thread.
   
   The use case is reported [here](/apache/logging-log4j2/commit/56436ad2176eac000d2821690e4373f097b76670#r44892412).
   
   > If we're restarting the background thread, why not simply catch ThreadDeath to avoid thread churn and potential bugs that result from the indirection?
   > 
   > Based on the original request when we began catching Error, I don't think the caller was stopping background threads (or using asynchronous logging at all) but rather calling Thread.stop on an application thread that happened to log. Using synchronous logging it would swallow the ThreadDeath error silently and allow the application thread to continue. I think we can allow ThreadDeath to be propagated, and even kill background threads -- if that's what the user wants and the security manager allows it, on their own heads be it!
   
   The major improvement this change set brings is that there are no smart whitelist of failures that are subject to handling. Anything above an `Exception`, we will not catch, which is the best practice while dealing with `Throwable`s. At the beginning, we were just catching `Exception`s. Then we figured there are certain `Error`s (e.g., `ExceptionInInitializerError`) that are harmless. Consequently we extended the catch block to `Throwable`s. Then somebody reported we shouldn't catch `ThreadDeath`. Evidently, we can't oversee all the potential corner cases while dealing with exceptions. Hence, in this patch, I just catch `Exception`s and respawn the thread on unexpected failures. I think, this is a way simpler and future-proof approach.
   
   > Also note that as written this PR only applies to the AsyncAppender, not the disruptor based fully async AsyncLoggerContextSelector or mixed sync/async logging.
   
   If we agree on the strategy employed here, I can extend it to wherever applicable. (The changes need to be duplicated on `master` anyway.)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org