Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-1874

Add ByteBufferDestionation.write(ByteBuffer) and write(byte[], int, int) methods and call them from TextEncoderHelper whenever possible

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.9.0
    • Component/s: None
    • Labels:
      None

      Description

      Existing ByteBufferDestination API: getByteBuffer() and drain() is designed so that synchronization couldn't be avoided. This doesn't allow to implement LOG4J2-928.

      Github PR: https://github.com/apache/logging-log4j2/pull/71

      Added methods: write(ByteBuffer data) and write(byte[] data, int offset, int length) are designed so that they should care about synchronization themselves, internally, if needed. They should also synchronize with possible concurrent users of the synchronized getByteBuffer() + drain() API. Nevertheless, it allows for ByteBufferDestination implementations to implement write() methods without lock-free.

      TextEncoderHelper (hence StringBuilderEncoder, which delegates it's logic to TextEncoderHelper) is changed so that it calls ByteBufferDestination.write() whenever possible. There is an expectation that most of encoded events fit the thread-local buffers, and write() could be called instead of writing to destination.getByteBuffer() with synchronization.

      The PR also includes a sanity improvement: uses ByteBuffer.arrayOffset() at some places.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          This PR is essentially a rebranded version of #70 which removes "bug" and "race condition" phrasing from the title and commit messages.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 This PR is essentially a rebranded version of #70 which removes "bug" and "race condition" phrasing from the title and commit messages.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/70

          @remkop I reviewed the existing code again and found that there is actually no race condition or bugs, if only one assumption is true: `ByteBufferDestination.drain()` returns ByteBuffer of capacity which is the same as the former `ByteBufferDestination.getByteBuffer()`'s capacity.

          However this refactoring is still needed, because it allows lock-free MemoryMappedFileManager. I've "rebraded" this PR as #71. The Jira issue with description is https://issues.apache.org/jira/browse/LOG4J2-1874.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/70 @remkop I reviewed the existing code again and found that there is actually no race condition or bugs, if only one assumption is true: `ByteBufferDestination.drain()` returns ByteBuffer of capacity which is the same as the former `ByteBufferDestination.getByteBuffer()`'s capacity. However this refactoring is still needed, because it allows lock-free MemoryMappedFileManager. I've "rebraded" this PR as #71. The Jira issue with description is https://issues.apache.org/jira/browse/LOG4J2-1874 .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @garydgregory this dangerous behaviour was not a bug, because it happened to always use byteBuffers which are created via `ByteBuffer.wrap(new byte[size])` or `ByteBuffer.allocate(size)`, but not `ByteBuffer.wrap(byteArray, off, len)`. But I think `ByteBuffer.array()` should always be paired with `byteBuffer.arrayOffset()` for sanity.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @garydgregory this dangerous behaviour was not a bug, because it happened to always use byteBuffers which are created via `ByteBuffer.wrap(new byte [size] )` or `ByteBuffer.allocate(size)`, but not `ByteBuffer.wrap(byteArray, off, len)`. But I think `ByteBuffer.array()` should always be paired with `byteBuffer.arrayOffset()` for sanity.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @leventov Thanks for the PR! I will review but it will take me some time.

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @leventov Thanks for the PR! I will review but it will take me some time.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @leventov Quick update: Started to look at the PR but did not get far yet. I'm taking my time because this is a fairly sensitive area. When converting the OutputStreamAppender and *FileAppenders to the ByteBufferDestination API for the garbage-free epic it took me a few tries to get it right. A complicating factor is that there may be users who subclassed any of these classes, and we don't want to break their stuff. At first glance your proposal does not affect that but I haven't spent enough time with it yet to be sure. (Did all the tests pass for you? I have some environment trouble and haven't been able to try yet.)

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @leventov Quick update: Started to look at the PR but did not get far yet. I'm taking my time because this is a fairly sensitive area. When converting the OutputStreamAppender and *FileAppenders to the ByteBufferDestination API for the garbage-free epic it took me a few tries to get it right. A complicating factor is that there may be users who subclassed any of these classes, and we don't want to break their stuff. At first glance your proposal does not affect that but I haven't spent enough time with it yet to be sure. (Did all the tests pass for you? I have some environment trouble and haven't been able to try yet.)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          > A complicating factor is that there may be users who subclassed any of these classes, and we don't want to break their stuff.

          If users could implement `ByteBufferDestination`, there is a problem, because new methods added to this interface.

          > Did all the tests pass for you?

          Yes, except obviously unrelated failures with something about TCP/network

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 > A complicating factor is that there may be users who subclassed any of these classes, and we don't want to break their stuff. If users could implement `ByteBufferDestination`, there is a problem, because new methods added to this interface. > Did all the tests pass for you? Yes, except obviously unrelated failures with something about TCP/network
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          I'm not so much concerned that users implemented ByteBufferDestination, but I'm more concerned that subclasses of OutputStreamManager and FileManager still work. For example, the new ByteBufferDestination.write(byte[], int, int) method needs to be renamed because it clashes with a pre-existing method (which was protected, not public). After that, could the existing write(byte[], int, int) method delegate to the new public one? Do they have the same concurrency semantics?

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 I'm not so much concerned that users implemented ByteBufferDestination, but I'm more concerned that subclasses of OutputStreamManager and FileManager still work. For example, the new ByteBufferDestination.write(byte[], int, int) method needs to be renamed because it clashes with a pre-existing method (which was protected, not public). After that, could the existing write(byte[], int, int) method delegate to the new public one? Do they have the same concurrency semantics?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop renamed. Yes, OutputStreamManager.write(byte[], int, int) has the same semantics.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop renamed. Yes, OutputStreamManager.write(byte[], int, int) has the same semantics.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          Apologies for the delay. I now have some time to start looking at this again.

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 Apologies for the delay. I now have some time to start looking at this again.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          I'm not done reviewing yet but I found two issues:

          • StringBuilderEncoder used to have ThreadLocal<CharBuffer>, ThreadLocal<ByteBuffer> and ThreadLocal<CharsetEncoder> fields, now has one ThreadLocal<ThreadLocalState> field. In the original code, the ThreadLocals only contains JDK classes, no custom (Log4j) classes. Where possible putting only JDK classes in ThreadLocals is preferable to avoid memory leaks in web containers: the Log4j classes may be loaded by a separate class loader which cannot be garbage collected if a thread pool threadlocal still has a reference to it. Can you revert this change?
          • The build consistently hangs in this core test: testCircularSuppressedExceptions(ThrowableProxyTest.java:391)

          I tried this three times with the same result, but when building with master the build succeeds. I haven't investigated further yet. Full stack trace follows below (notice how it stops halfway a stack trace, as if a buffer is full...)

          ```
          23:13:06.369 [main] ERROR org.apache.logging.log4j.core.impl.ThrowableProxyTest - Error
          java.lang.Exception: null
          at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:391) [test-classes/:?]
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_131]
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_131]
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_131]
          at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_131]
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12]
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUnitCoreWrapper.java:107) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:83) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:75) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreProvider.invoke(JUnitCoreProvider.java:161) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290) [surefire-booter-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242) [surefire-booter-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121) [surefire-booter-2.19.1.jar:2.19.1]
          Suppressed: java.lang.Exception
          at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:392) [test-classes/:?]
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_131]
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_131]
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_131]
          at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_131]
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12]
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUnitCoreWrapper.java:107) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:83) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:75) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreProvider.invoke(JUnitCoreProvider.java:161) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290) [surefire-booter-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242) [surefire-booter-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121) [surefire-booter-2.19.1.jar:2.19.1]
          Suppressed: java.lang.Exception
          at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:391) [test-classes/:?]
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_131]
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_131]
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_131]
          at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_131]
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12]
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12]
          at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12]
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12]
          at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
          at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
          at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1]
          at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUni
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 I'm not done reviewing yet but I found two issues: StringBuilderEncoder used to have ThreadLocal<CharBuffer>, ThreadLocal<ByteBuffer> and ThreadLocal<CharsetEncoder> fields, now has one ThreadLocal<ThreadLocalState> field. In the original code, the ThreadLocals only contains JDK classes, no custom (Log4j) classes. Where possible putting only JDK classes in ThreadLocals is preferable to avoid memory leaks in web containers: the Log4j classes may be loaded by a separate class loader which cannot be garbage collected if a thread pool threadlocal still has a reference to it. Can you revert this change? The build consistently hangs in this core test: testCircularSuppressedExceptions(ThrowableProxyTest.java:391) I tried this three times with the same result, but when building with master the build succeeds. I haven't investigated further yet. Full stack trace follows below (notice how it stops halfway a stack trace, as if a buffer is full...) ``` 23:13:06.369 [main] ERROR org.apache.logging.log4j.core.impl.ThrowableProxyTest - Error java.lang.Exception: null at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:391) [test-classes/:?] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~ [?:1.8.0_131] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~ [?:1.8.0_131] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~ [?:1.8.0_131] at java.lang.reflect.Method.invoke(Method.java:498) ~ [?:1.8.0_131] at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12] at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12] at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12] at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUnitCoreWrapper.java:107) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:83) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:75) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreProvider.invoke(JUnitCoreProvider.java:161) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290) [surefire-booter-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242) [surefire-booter-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121) [surefire-booter-2.19.1.jar:2.19.1] Suppressed: java.lang.Exception at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:392) [test-classes/:?] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~ [?:1.8.0_131] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~ [?:1.8.0_131] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~ [?:1.8.0_131] at java.lang.reflect.Method.invoke(Method.java:498) ~ [?:1.8.0_131] at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12] at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12] at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12] at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUnitCoreWrapper.java:107) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:83) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:75) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreProvider.invoke(JUnitCoreProvider.java:161) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290) [surefire-booter-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242) [surefire-booter-2.19.1.jar:2.19.1] at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121) [surefire-booter-2.19.1.jar:2.19.1] Suppressed: java.lang.Exception at org.apache.logging.log4j.core.impl.ThrowableProxyTest.testCircularSuppressedExceptions(ThrowableProxyTest.java:391) [test-classes/:?] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~ [?:1.8.0_131] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~ [?:1.8.0_131] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~ [?:1.8.0_131] at java.lang.reflect.Method.invoke(Method.java:498) ~ [?:1.8.0_131] at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12] at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12] at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12] at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:128) [junit-4.12.jar:4.12] at org.junit.runners.Suite.runChild(Suite.java:27) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12] at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12] at org.apache.maven.surefire.junitcore.JUnitCore.run(JUnitCore.java:55) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:137) [surefire-junit47-2.19.1.jar:2.19.1] at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUni ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop thanks for review. Replaced custom class in ThreadLocal with `Object[]`. Fixed a bug in `TextEncoderHelper`, `ThrowableProxyTest.testCircularSuppressedExceptions()` passes now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop thanks for review. Replaced custom class in ThreadLocal with `Object[]`. Fixed a bug in `TextEncoderHelper`, `ThrowableProxyTest.testCircularSuppressedExceptions()` passes now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jvz commented on a diff in the pull request:

          https://github.com/apache/logging-log4j2/pull/71#discussion_r115505200

          — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java —
          @@ -32,9 +32,14 @@
          public class StringBuilderEncoder implements Encoder<StringBuilder> {

          private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024;

          • private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>();
            + /**
            + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer
              • End diff –

          So why can't you use three separate thread locals?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jvz commented on a diff in the pull request: https://github.com/apache/logging-log4j2/pull/71#discussion_r115505200 — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java — @@ -32,9 +32,14 @@ public class StringBuilderEncoder implements Encoder<StringBuilder> { private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024; private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>(); + /** + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer End diff – So why can't you use three separate thread locals?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on a diff in the pull request:

          https://github.com/apache/logging-log4j2/pull/71#discussion_r115512718

          — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java —
          @@ -32,9 +32,14 @@
          public class StringBuilderEncoder implements Encoder<StringBuilder> {

          private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024;

          • private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>();
            + /**
            + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer
              • End diff –

          @jvz I can. Using just one is an optimization - ThreadLocalMap is polluted less, ThreadLocalMap.get() is called only once instead of three times.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on a diff in the pull request: https://github.com/apache/logging-log4j2/pull/71#discussion_r115512718 — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java — @@ -32,9 +32,14 @@ public class StringBuilderEncoder implements Encoder<StringBuilder> { private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024; private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>(); + /** + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer End diff – @jvz I can. Using just one is an optimization - ThreadLocalMap is polluted less, ThreadLocalMap.get() is called only once instead of three times.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on a diff in the pull request:

          https://github.com/apache/logging-log4j2/pull/71#discussion_r115515373

          — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java —
          @@ -32,9 +32,14 @@
          public class StringBuilderEncoder implements Encoder<StringBuilder> {

          private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024;

          • private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>();
          • private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>();
            + /**
            + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer
              • End diff –

          Updated a comment in code

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on a diff in the pull request: https://github.com/apache/logging-log4j2/pull/71#discussion_r115515373 — Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/StringBuilderEncoder.java — @@ -32,9 +32,14 @@ public class StringBuilderEncoder implements Encoder<StringBuilder> { private static final int DEFAULT_BYTE_BUFFER_SIZE = 8 * 1024; private final ThreadLocal<CharBuffer> charBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<ByteBuffer> byteBufferThreadLocal = new ThreadLocal<>(); private final ThreadLocal<CharsetEncoder> charsetEncoderThreadLocal = new ThreadLocal<>(); + /** + * This ThreadLocal uses raw and inconvenient Object[] to store three heterogeneous objects (CharEncoder, CharBuffer End diff – Updated a comment in code
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @leventov Thanks for the quick turnaround. Today I only had time to check that the tests pass, but I ran `mvn clean install` twice and both times it failed here:

          ```
          [ERROR] Tests run: 23, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.075 s <<< FAILURE! - in org.apache.logging.log4j.core.layout.PatternLayoutTest
          [ERROR] testPatternSelector(org.apache.logging.log4j.core.layout.PatternLayoutTest) Time elapsed: 0.056 s <<< FAILURE!
          java.lang.AssertionError:
          Unexpected result: 2017-05-10 00:17:21,574 TRACE [main]: ====== PatternLayoutTest.testPatternSelector:259 entry ======

          at org.apache.logging.log4j.core.layout.PatternLayoutTest.testPatternSelector(PatternLayoutTest.java:261)
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @leventov Thanks for the quick turnaround. Today I only had time to check that the tests pass, but I ran `mvn clean install` twice and both times it failed here: ``` [ERROR] Tests run: 23, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.075 s <<< FAILURE! - in org.apache.logging.log4j.core.layout.PatternLayoutTest [ERROR] testPatternSelector(org.apache.logging.log4j.core.layout.PatternLayoutTest) Time elapsed: 0.056 s <<< FAILURE! java.lang.AssertionError: Unexpected result: 2017-05-10 00:17:21,574 TRACE [main] : ====== PatternLayoutTest.testPatternSelector:259 entry ====== at org.apache.logging.log4j.core.layout.PatternLayoutTest.testPatternSelector(PatternLayoutTest.java:261) ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop this test used to depend on specific line numbers in this test. Made it more generic.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop this test used to depend on specific line numbers in this test. Made it more generic.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          Do all tests pass when you run `mvn clean install` on your machine? (may take ~20 minutes)

          On Wednesday, May 10, 2017 12:34 AM, Roman Leventov <notifications@github.com> wrote:

          @remkop this test used to depend on specific line numbers in this test. Made it more generic.—
          You are receiving this because you were mentioned.
          Reply to this email directly, view it on GitHub, or mute the thread.

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 Do all tests pass when you run `mvn clean install` on your machine? (may take ~20 minutes) On Wednesday, May 10, 2017 12:34 AM, Roman Leventov <notifications@github.com> wrote: @remkop this test used to depend on specific line numbers in this test. Made it more generic.— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop I did that for log4j-core only. Yes, it passes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop I did that for log4j-core only. Yes, it passes.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop any extra comments on this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop any extra comments on this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          I got distracted by other things but still reviewing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/71 I got distracted by other things but still reviewing.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov commented on the issue:

          https://github.com/apache/logging-log4j2/pull/71

          @remkop thanks for commitment

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov commented on the issue: https://github.com/apache/logging-log4j2/pull/71 @remkop thanks for commitment
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          I finally was able to find a long enough block of uninterrupted time to review the PR. Thank you for your patience!

          I found a few minor things during code review:
          I believe TextEncoderHelper#writeEncodedText() line 38 has a small bug:

          private static void writeEncodedText(CharsetEncoder charsetEncoder, CharBuffer charBuf, ByteBuffer byteBuf, ByteBufferDestination destination, CoderResult result) {
              if (!result.isUnderflow()) {
                  writeChunkedEncodedText(charsetEncoder, charBuf, destination, byteBuf, result);
                  return;
              }
              charsetEncoder.flush(byteBuf); // <-- (line 38) the return value from flush() is ignored
              if (!result.isUnderflow()) {
              ...
          

          I believe the intention was the code below, is that correct?

          result = charsetEncoder.flush(byteBuf);
          

          Also, TextEncoderHelper#drainIfByteBufferFull: calling this method may result in the ByteBufferDestination swapping its ByteBuffer object for another object. Callers must synchronize on ByteBufferDestination. Currently all callers do this correctly, so there is no problem. However to protect against mistakes in future modifications, and for clarity, I propose to let the relevant portion of this method explicitly synchronize on ByteBufferDestination:

          private static ByteBuffer drainIfByteBufferFull(ByteBufferDestination destination, ByteBuffer temp, CoderResult result) {
              if (result.isOverflow()) { // byte buffer full
                  synchronized (destination) {
                      ByteBuffer destinationBuffer = destination.getByteBuffer();
                      if (destinationBuffer != temp) {
                          temp.flip();
                          ByteBufferDestinationHelper.writeToUnsynchronized(temp, destination);
                          temp.clear();
                          return destination.getByteBuffer();
                      } else {
                          return destination.drain(destinationBuffer);
                      }
                  }
              } else {
                  return temp;
              }
          }
          

          If you agree with the above changes just comment, no need to update the pull request.

          I will step through a few tests in the debugger for final confirmation and if no issues arise I plan to merge the PR, hopefully in the next few days.

          Show
          remkop@yahoo.com Remko Popma added a comment - I finally was able to find a long enough block of uninterrupted time to review the PR. Thank you for your patience! I found a few minor things during code review: I believe TextEncoderHelper#writeEncodedText() line 38 has a small bug: private static void writeEncodedText(CharsetEncoder charsetEncoder, CharBuffer charBuf, ByteBuffer byteBuf, ByteBufferDestination destination, CoderResult result) { if (!result.isUnderflow()) {         writeChunkedEncodedText(charsetEncoder, charBuf, destination, byteBuf, result);         return ;     }     charsetEncoder.flush(byteBuf); // <-- (line 38) the return value from flush() is ignored     if (!result.isUnderflow()) {     ... I believe the intention was the code below, is that correct? result = charsetEncoder.flush(byteBuf); Also, TextEncoderHelper#drainIfByteBufferFull : calling this method may result in the ByteBufferDestination swapping its ByteBuffer object for another object. Callers must synchronize on ByteBufferDestination. Currently all callers do this correctly, so there is no problem. However to protect against mistakes in future modifications, and for clarity, I propose to let the relevant portion of this method explicitly synchronize on ByteBufferDestination: private static ByteBuffer drainIfByteBufferFull(ByteBufferDestination destination, ByteBuffer temp, CoderResult result) {     if (result.isOverflow()) { // byte buffer full         synchronized (destination) {             ByteBuffer destinationBuffer = destination.getByteBuffer();             if (destinationBuffer != temp) {                 temp.flip();                 ByteBufferDestinationHelper.writeToUnsynchronized(temp, destination);                 temp.clear();                 return destination.getByteBuffer();             } else {                 return destination.drain(destinationBuffer);             }         }     } else {         return temp;     } } If you agree with the above changes just comment, no need to update the pull request. I will step through a few tests in the debugger for final confirmation and if no issues arise I plan to merge the PR, hopefully in the next few days.
          Hide
          leventov Roman Leventov added a comment -

          Thanks for review!

          1) Yes, it's a bug
          2) OK, and I would also add a comment on that synchronized block, that it is unnecessary at the moment, but added for safety.

          > If you agree with the above changes just comment, no need to update the pull request.
          You mean that you could fix it yourself when merging?

          Show
          leventov Roman Leventov added a comment - Thanks for review! 1) Yes, it's a bug 2) OK, and I would also add a comment on that synchronized block, that it is unnecessary at the moment, but added for safety. > If you agree with the above changes just comment, no need to update the pull request. You mean that you could fix it yourself when merging?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d92cc27d3cdc0a5d7e7f8c519abf592c547e015b in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=d92cc27 ]

          LOG4J2-1874 add writeBytes methods to ByteBufferDestination

          Added methods ::writeBytes(ByteBuffer) and ::writeBytes(byte[], int, int) to ByteBufferDestination interface and use these methods in TextEncoderHelper where possible to prepare for future enhancements to reduce lock contention.
          Closes GitHub pull request #71

          Show
          jira-bot ASF subversion and git services added a comment - Commit d92cc27d3cdc0a5d7e7f8c519abf592c547e015b in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=d92cc27 ] LOG4J2-1874 add writeBytes methods to ByteBufferDestination Added methods ::writeBytes(ByteBuffer) and ::writeBytes(byte[], int, int) to ByteBufferDestination interface and use these methods in TextEncoderHelper where possible to prepare for future enhancements to reduce lock contention. Closes GitHub pull request #71
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          Thank you for your patience.
          Pull request #71, with some minor changes, has been merged into master.
          Please verify and close.

          Show
          remkop@yahoo.com Remko Popma added a comment - Thank you for your patience. Pull request #71, with some minor changes, has been merged into master. Please verify and close.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6c8f0d6bc299066d3cabc2726130faa2cb560555 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=6c8f0d6 ]

          LOG4J2-1874 javadoc enhancements

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6c8f0d6bc299066d3cabc2726130faa2cb560555 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=6c8f0d6 ] LOG4J2-1874 javadoc enhancements
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user leventov closed the pull request at:

          https://github.com/apache/logging-log4j2/pull/71

          Show
          githubbot ASF GitHub Bot added a comment - Github user leventov closed the pull request at: https://github.com/apache/logging-log4j2/pull/71

            People

            • Assignee:
              remkop@yahoo.com Remko Popma
              Reporter:
              leventov Roman Leventov
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development