vy commented on pull request #452: URL: https://github.com/apache/logging-log4j2/pull/452#issuecomment-752739475
> I hope I'm not coming off too strong here Not at all! Somebody is thinking along with me free of charge! What else I can ask for? I am really grateful that you take time for this. :bow: > In my proposal above, the AsyncAppender would catch it and warn/ignore as it catches all throwables, but the AppenderControl would allow ThreadDeath to propagate, bringing back 2.13.x behavior. The difference is that the AppenderControl can throw errors again, but background threads must be robust against them. But once `AsyncAppender` catches the `ThreadDeath`, it is basically dead. Who is gonna restart that thread? > This PR makes the AsyncAppender robust against Error by transitioning to another thread, but I'm not sure why we need to change threads instead of allowing the original thread to continue. Because the original thread is dead as indicated by `ThreadDeath`? > > In summary, I am really reluctant to add any kind of `catch (Throwable ...)` clauses. > > I think the reason this strikes me as odd is that this implementation is already using a `catch (Throwable ...)`, but without writing it directly. We're using an uncaught exception handler to catch `Throwable` and handle it by logging to the statuslogger, and start another thread. If we explicitly write the try/catch instead, we can produce identical behavior without creating new thread Indeed the code is still possessing a `catch (Throwable ...)` behavior, but we delegate handling of non-`Exception` throwables to the JDK, rather than putting a plaster to every new hole we discover. > The `catch Throwable` occurs outside log4j2, in the disruptor implementation: > > https://github.com/LMAX-Exchange/disruptor/blob/c1ea0d4a6d6144d338a9ff8cb0d46f2a7038b968/src/main/java/com/lmax/disruptor/BatchEventProcessor.java#L185-L190 Really ~weird~ interesting... I still think `catch (Throwable ...)` is a time bomb waiting to be exploited by a bug report. You are handling `OutOfMemoryError`s, `ThreadDeath`s, etc. I will try poking somebody from Disruptor to comment on this. ---------------------------------------------------------------- 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: [email protected]
