jacobrshields opened a new pull request #488: URL: https://github.com/apache/logging-log4j2/pull/488
[LOG4J2-2816](https://issues.apache.org/jira/browse/LOG4J2-2816) # Problem If `RingBufferLogEventTranslator#translateTo` throws an exception for any reason, Disruptor's `RingBuffer#translateAndPublish` will still publish the sequence in a `finally` block ([source](https://github.com/LMAX-Exchange/disruptor/blob/ca35bc40eb7f834050793137b5996a0921173e2d/src/main/java/com/lmax/disruptor/RingBuffer.java#L958-L968)). In such a case, the "untranslated" and unpopulated event will later be consumed by `RingBufferLogEventHandler`, since its sequence was published. However, the event will be missing all values, including `asyncLogger`. This causes a `NullPointerException` to be thrown during event handling in `RingBufferLogEvent#execute`: ```text AsyncLogger error handling event seq=0, value='org.apache.logging.log4j.core.async.RingBufferLogEvent@7bb6d06c': java.lang.NullPointerException: null java.lang.NullPointerException at org.apache.logging.log4j.core.async.RingBufferLogEvent.execute(RingBufferLogEvent.java:154) at org.apache.logging.log4j.core.async.RingBufferLogEventHandler.onEvent(RingBufferLogEventHandler.java:46) at org.apache.logging.log4j.core.async.RingBufferLogEventHandler.onEvent(RingBufferLogEventHandler.java:29) at com.lmax.disruptor.BatchEventProcessor.processEvents(BatchEventProcessor.java:168) at com.lmax.disruptor.BatchEventProcessor.run(BatchEventProcessor.java:125) at java.lang.Thread.run(Thread.java:748) ``` # Solution Log4j needs to handle the case where an exception is thrown by `RingBufferLogEventTranslator#translateTo`. The Disruptor documentation makes it clear that once a slot is claimed in the ring buffer, the sequence _must_ be published. Otherwise the state of the Disruptor can be corrupted ([source](https://lmax-exchange.github.io/disruptor/user-guide/index.html#_publishing_using_the_legacy_api)). That is why `RingBuffer#translateAndPublish` is designed the way it is, and why its usage is recommended over more manual methods. In order to handle such exceptions, then, it seems like the `EventHandler` must be responsible for checking that the event it is handling is sufficiently populated. Such an approach is also suggested by [this Disruptor GitHub issue discussion](https://github.com/LMAX-Exchange/disruptor/issues/244#issuecomment-437814712). This PR implements that approach by adding an `isPopulated` property to `RingBufferLogEvent`. # Tests The new test in `AsyncLoggerEventTranslationExceptionTest` fails on `release-2.x` and passes on this branch. -- 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]
