[
https://issues.apache.org/jira/browse/LOG4J2-1874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16049257#comment-16049257
]
Remko Popma commented on LOG4J2-1874:
-------------------------------------
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:
{code}
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()) {
...
{code}
I believe the intention was the code below, is that correct?
{code}
result = charsetEncoder.flush(byteBuf);
{code}
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:
{code}
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;
}
}
{code}
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.
> Add ByteBufferDestionation.write(ByteBuffer) and write(byte[], int, int)
> methods and call them from TextEncoderHelper whenever possible
> ---------------------------------------------------------------------------------------------------------------------------------------
>
> Key: LOG4J2-1874
> URL: https://issues.apache.org/jira/browse/LOG4J2-1874
> Project: Log4j 2
> Issue Type: Improvement
> Reporter: Roman Leventov
> Assignee: Remko Popma
>
> 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.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
