You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/05/02 20:45:02 UTC
[GitHub] [logging-log4j2] jacobrshields opened a new pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions
jacobrshields opened a new pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487
# 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 `master` 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:
users@infra.apache.org
[GitHub] [logging-log4j2] jacobrshields edited a comment on pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions (3.x)
Posted by GitBox <gi...@apache.org>.
jacobrshields edited a comment on pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487#issuecomment-830916668
Thanks for the quick response, Gary. To clarify, the build passes locally. The failure is on the CI build. I would expect the CI build to work independently of my own Maven toolchain XML file.
Regardless, I did want this fix for the 2.x line. I applied this change to `release-2.x` in PR #488. Thanks for pointing this out!
--
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
[GitHub] [logging-log4j2] jacobrshields commented on pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions
Posted by GitBox <gi...@apache.org>.
jacobrshields commented on pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487#issuecomment-830869512
CI is failing with this error. Any ideas how to fix this?
```
[ERROR] javac: invalid flag: --release
```
--
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
[GitHub] [logging-log4j2] jacobrshields commented on pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions (3.x)
Posted by GitBox <gi...@apache.org>.
jacobrshields commented on pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487#issuecomment-830916668
Thanks for the quick response, Gary. To clarify, the build passes locally. The failure is on the CI build. I would expect the CI build to work regardless of my own Maven toolchain XML file.
Regardless, I did want this fix for the 2.x line. I applied this change to `release-2.x` in PR #488. Thanks for pointing this out!
--
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
[GitHub] [logging-log4j2] garydgregory commented on pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487#issuecomment-830871192
If you want this fix for the 2.x line, master is the wrong branch as it will become 3.0.0. The 2.x line is from release-2.x. Check your Maven toolchain XML file, it might be the case that you are mismatched with what JDKs we expect to be configured.
--
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
[GitHub] [logging-log4j2] jacobrshields closed pull request #487: LOG4J2-2816: Handle Disruptor event translation exceptions (3.x)
Posted by GitBox <gi...@apache.org>.
jacobrshields closed pull request #487:
URL: https://github.com/apache/logging-log4j2/pull/487
--
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