From 9fb47f5dcf3848320991c7a2cf602eb325893a42 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 02:08:45 +0200 Subject: [PATCH 01/39] Add a test that reproduces a bug in checksum calculation --- ...ompositeByteBufUnwrapBugReproduceTest.java | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java new file mode 100644 index 00000000000..fbe3e87074d --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -0,0 +1,165 @@ +package org.apache.bookkeeper.proto.checksum; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import com.scurrilous.circe.checksum.IntHash; +import com.scurrilous.circe.checksum.Java8IntHash; +import com.scurrilous.circe.checksum.Java9IntHash; +import com.scurrilous.circe.checksum.JniIntHash; +import com.scurrilous.circe.crc.Sse42Crc32C; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufAllocator; +import io.netty.buffer.ByteBufUtil; +import io.netty.buffer.CompositeByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.util.ReferenceCounted; +import java.util.Arrays; +import java.util.Collection; +import org.apache.bookkeeper.proto.BookieProtoEncoding; +import org.apache.bookkeeper.proto.BookieProtocol; +import org.apache.bookkeeper.util.ByteBufList; +import org.apache.commons.lang3.RandomUtils; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +/** + * Reproduces a bug in the checksum calculation when the payload is + * a CompositeByteBuf and this buffer has a reader index state other than 0. + * The reader index state gets lost in the unwrapping process. + * + * There are at least 2 different bugs. One that occurs when the + * payload is >= BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD and the other when + * it's < BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD. + * This test covers both useV2Protocol=true and useV2Protocol=false since the bug is triggered differently. + */ +@RunWith(Parameterized.class) +public class CompositeByteBufUnwrapBugReproduceTest { + final byte[] testPayLoad; + final int bufferPrefixLength; + private final boolean useV2Protocol; + + @Parameterized.Parameters + public static Collection testScenarios() { + return Arrays.asList(new Object[][] { + {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD - 1, true}, + {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD - 1, false}, + {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, true}, + {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, false} + }); + } + + public CompositeByteBufUnwrapBugReproduceTest(int payloadSize, boolean useV2Protocol) { + this.testPayLoad = createTestPayLoad(payloadSize); + this.bufferPrefixLength = payloadSize / 7; + this.useV2Protocol = useV2Protocol; + } + + private static byte[] createTestPayLoad(int payloadSize) { + byte[] payload = new byte[payloadSize]; + for (int i = 0; i < payloadSize; i++) { + payload[i] = (byte) i; + } + return payload; + } + + + /** + * A DigestManager that uses the given IntHash implementation for testing. + */ + static class TestIntHashDigestManager extends DigestManager { + private final IntHash intHash; + + public TestIntHashDigestManager(IntHash intHash, long ledgerId, boolean useV2Protocol, ByteBufAllocator allocator) { + super(ledgerId, useV2Protocol, allocator); + this.intHash = intHash; + } + + @Override + int getMacCodeLength() { + return 4; + } + + @Override + boolean isInt32Digest() { + return true; + } + + @Override + void populateValueAndReset(int digest, ByteBuf buf) { + buf.writeInt(digest); + } + + @Override + int update(int digest, ByteBuf data, int offset, int len) { + return intHash.resume(digest, data, offset, len); + } + } + + @Test + public void shouldCalculateChecksumForCompositeBuffer() { + ByteBuf testPayload = Unpooled.wrappedBuffer(testPayLoad); + byte[] referenceOutput = computeDigestAndPackageForSending(new Java8IntHash(), testPayload.retainedDuplicate()); + assertDigestAndPackageMatchesReference(new Java8IntHash(), testPayload, referenceOutput); + assertDigestAndPackageMatchesReference(new Java9IntHash(), testPayload, referenceOutput); + if (Sse42Crc32C.isSupported()) { + assertDigestAndPackageMatchesReference(new JniIntHash(), testPayload, referenceOutput); + } + testPayload.release(); + } + + private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf payload, byte[] referenceOutput) { + byte[] output1 = computeDigestAndPackageForSending(intHash, payload.retainedDuplicate()); + assertArrayEquals(referenceOutput, output1); + + // wrap the payload in a CompositeByteBuf which contains another buffer where the payload contains a prefix + // the reader index is set past the prefix + ByteBuf payload2 = wrapWithPrefixAndCompositeByteBufWithReaderIndexState(payload.retainedDuplicate()); + // this validates that the readable bytes in payload2 match the TEST_PAYLOAD content + assertArrayEquals(ByteBufUtil.getBytes(payload2.duplicate()), testPayLoad); + + byte[] output2 = computeDigestAndPackageForSending(intHash, payload2); + assertArrayEquals(output1, output2); + } + + private byte[] computeDigestAndPackageForSending(IntHash intHash, ByteBuf data) { + DigestManager digestManager = new TestIntHashDigestManager(intHash, 1, useV2Protocol, ByteBufAllocator.DEFAULT); + ReferenceCounted packagedBuffer = + digestManager.computeDigestAndPackageForSending(1, 0, data.readableBytes(), data, + MacDigestManager.EMPTY_LEDGER_KEY, BookieProtocol.FLAG_NONE); + return packagedBufferToBytes(packagedBuffer); + } + + ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload) { + // create a new buffer with a prefix and the actual payload + ByteBuf prefixedPayload = ByteBufAllocator.DEFAULT.buffer(bufferPrefixLength + payload.readableBytes()); + prefixedPayload.writeBytes(RandomUtils.nextBytes(bufferPrefixLength)); + prefixedPayload.writeBytes(payload); + + // wrap the buffer in a composite buffer + CompositeByteBuf outerComposite = ByteBufAllocator.DEFAULT.compositeBuffer(); + outerComposite.addComponent(true, prefixedPayload); + + // set reader index state. this is the state that gets lost in the unwrapping process + outerComposite.readerIndex(bufferPrefixLength); + + return outerComposite; + } + + private static byte[] packagedBufferToBytes(ReferenceCounted packagedBuffer) { + byte[] output; + if (packagedBuffer instanceof ByteBufList) { + ByteBufList bufList = (ByteBufList) packagedBuffer; + output = new byte[bufList.readableBytes()]; + bufList.getBytes(output); + for (int i = 0; i < bufList.size(); i++) { + bufList.getBuffer(i).release(); + } + } else if (packagedBuffer instanceof ByteBuf) { + output = ByteBufUtil.getBytes((ByteBuf) packagedBuffer); + packagedBuffer.release(); + } else { + throw new RuntimeException("Unexpected type: " + packagedBuffer.getClass()); + } + return output; + } +} \ No newline at end of file From 0707710e209ec43be02ef01071f57ce917362a0f Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 30 Jan 2024 08:35:38 +0200 Subject: [PATCH 02/39] Revert "Fixed unnecessary copy to heap (#2701)" changes to ByteBufList This partially reverts commit 3c9c7102538909fd3764ea7314e7618d6d9458fd. --- .../apache/bookkeeper/util/ByteBufList.java | 36 ++----------------- .../bookkeeper/util/ByteBufListTest.java | 34 ------------------ 2 files changed, 3 insertions(+), 67 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufList.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufList.java index 324588d852b..b363e4da636 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufList.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufList.java @@ -22,7 +22,6 @@ import com.google.common.annotations.VisibleForTesting; import io.netty.buffer.ByteBuf; -import io.netty.buffer.CompositeByteBuf; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelHandler.Sharable; import io.netty.channel.ChannelHandlerContext; @@ -133,43 +132,14 @@ public static ByteBufList get() { * Append a {@link ByteBuf} at the end of this {@link ByteBufList}. */ public void add(ByteBuf buf) { - final ByteBuf unwrapped = buf.unwrap() != null && buf.unwrap() instanceof CompositeByteBuf - ? buf.unwrap() : buf; - ReferenceCountUtil.retain(unwrapped); - ReferenceCountUtil.release(buf); - - if (unwrapped instanceof CompositeByteBuf) { - ((CompositeByteBuf) unwrapped).forEach(b -> { - ReferenceCountUtil.retain(b); - buffers.add(b); - }); - ReferenceCountUtil.release(unwrapped); - } else { - buffers.add(unwrapped); - } + buffers.add(buf); } /** * Prepend a {@link ByteBuf} at the beginning of this {@link ByteBufList}. */ public void prepend(ByteBuf buf) { - // don't unwrap slices - final ByteBuf unwrapped = buf.unwrap() != null && buf.unwrap() instanceof CompositeByteBuf - ? buf.unwrap() : buf; - ReferenceCountUtil.retain(unwrapped); - ReferenceCountUtil.release(buf); - - if (unwrapped instanceof CompositeByteBuf) { - CompositeByteBuf composite = (CompositeByteBuf) unwrapped; - for (int i = composite.numComponents() - 1; i >= 0; i--) { - ByteBuf b = composite.component(i); - ReferenceCountUtil.retain(b); - buffers.add(0, b); - } - ReferenceCountUtil.release(unwrapped); - } else { - buffers.add(0, unwrapped); - } + buffers.add(0, buf); } /** @@ -285,7 +255,7 @@ public ByteBufList retain() { @Override protected void deallocate() { for (int i = 0; i < buffers.size(); i++) { - ReferenceCountUtil.release(buffers.get(i)); + buffers.get(i).release(); } buffers.clear(); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/ByteBufListTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/ByteBufListTest.java index ac7aca77226..88de17d0a9d 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/ByteBufListTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/ByteBufListTest.java @@ -23,7 +23,6 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; -import io.netty.buffer.CompositeByteBuf; import io.netty.buffer.PooledByteBufAllocator; import io.netty.buffer.Unpooled; import io.netty.channel.Channel; @@ -87,39 +86,6 @@ public void testDouble() throws Exception { assertEquals(b2.refCnt(), 0); } - @Test - public void testComposite() throws Exception { - ByteBuf b1 = PooledByteBufAllocator.DEFAULT.heapBuffer(128, 128); - b1.writerIndex(b1.capacity()); - ByteBuf b2 = PooledByteBufAllocator.DEFAULT.heapBuffer(128, 128); - b2.writerIndex(b2.capacity()); - - CompositeByteBuf composite = PooledByteBufAllocator.DEFAULT.compositeBuffer(); - composite.addComponent(b1); - composite.addComponent(b2); - - ByteBufList buf = ByteBufList.get(composite); - - // composite is unwrapped into two parts - assertEquals(2, buf.size()); - // and released - assertEquals(composite.refCnt(), 0); - - assertEquals(256, buf.readableBytes()); - assertEquals(b1, buf.getBuffer(0)); - assertEquals(b2, buf.getBuffer(1)); - - assertEquals(buf.refCnt(), 1); - assertEquals(b1.refCnt(), 1); - assertEquals(b2.refCnt(), 1); - - buf.release(); - - assertEquals(buf.refCnt(), 0); - assertEquals(b1.refCnt(), 0); - assertEquals(b2.refCnt(), 0); - } - @Test public void testClone() throws Exception { ByteBuf b1 = PooledByteBufAllocator.DEFAULT.heapBuffer(128, 128); From fe9301da83b27bb6e40e9466b17d1a8af8b26ff6 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 30 Jan 2024 08:46:29 +0200 Subject: [PATCH 03/39] Remove CompositeBuffer unwrapping in DigestManager --- .../proto/checksum/DigestManager.java | 45 +++---------------- 1 file changed, 6 insertions(+), 39 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index eab33945b1e..71cef2e3e7b 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -20,10 +20,8 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.ByteBufUtil; -import io.netty.buffer.CompositeByteBuf; import io.netty.buffer.PooledByteBufAllocator; import io.netty.buffer.Unpooled; -import io.netty.util.ReferenceCountUtil; import io.netty.util.ReferenceCounted; import io.netty.util.concurrent.FastThreadLocal; import java.security.GeneralSecurityException; @@ -136,22 +134,7 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long // Compute checksum over the headers int digest = update(0, buf, buf.readerIndex(), buf.readableBytes()); - - // don't unwrap slices - final ByteBuf unwrapped = data.unwrap() != null && data.unwrap() instanceof CompositeByteBuf - ? data.unwrap() : data; - ReferenceCountUtil.retain(unwrapped); - ReferenceCountUtil.safeRelease(data); - - if (unwrapped instanceof CompositeByteBuf) { - CompositeByteBuf cbb = (CompositeByteBuf) unwrapped; - for (int i = 0; i < cbb.numComponents(); i++) { - ByteBuf b = cbb.component(i); - digest = update(digest, b, b.readerIndex(), b.readableBytes()); - } - } else { - digest = update(digest, unwrapped, unwrapped.readerIndex(), unwrapped.readableBytes()); - } + digest = update(digest, data, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, buf); @@ -159,11 +142,11 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long buf.readerIndex(0); if (isSmallEntry) { - buf.writeBytes(unwrapped, unwrapped.readerIndex(), unwrapped.readableBytes()); - unwrapped.release(); + buf.writeBytes(data, data.readerIndex(), data.readableBytes()); + data.release(); return buf; } else { - return ByteBufList.get(buf, unwrapped); + return ByteBufList.get(buf, data); } } @@ -176,25 +159,9 @@ private ByteBufList computeDigestAndPackageForSendingV3(long entryId, long lastA headersBuffer.writeLong(length); int digest = update(0, headersBuffer, 0, METADATA_LENGTH); - - // don't unwrap slices - final ByteBuf unwrapped = data.unwrap() != null && data.unwrap() instanceof CompositeByteBuf - ? data.unwrap() : data; - ReferenceCountUtil.retain(unwrapped); - ReferenceCountUtil.release(data); - - if (unwrapped instanceof CompositeByteBuf) { - CompositeByteBuf cbb = ((CompositeByteBuf) unwrapped); - for (int i = 0; i < cbb.numComponents(); i++) { - ByteBuf b = cbb.component(i); - digest = update(digest, b, b.readerIndex(), b.readableBytes()); - } - } else { - digest = update(digest, unwrapped, unwrapped.readerIndex(), unwrapped.readableBytes()); - } + digest = update(digest, data, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, headersBuffer); - - return ByteBufList.get(headersBuffer, unwrapped); + return ByteBufList.get(headersBuffer, data); } /** From 532de4f2baf1b87644b514092427db926bc03445 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 30 Jan 2024 08:57:41 +0200 Subject: [PATCH 04/39] Rename update -> internalUpdate so that unwrapping logic could be added to update --- .../bookkeeper/proto/checksum/CRC32CDigestManager.java | 2 +- .../bookkeeper/proto/checksum/CRC32DigestManager.java | 2 +- .../org/apache/bookkeeper/proto/checksum/DigestManager.java | 6 +++++- .../bookkeeper/proto/checksum/DummyDigestManager.java | 2 +- .../apache/bookkeeper/proto/checksum/MacDigestManager.java | 2 +- .../checksum/CompositeByteBufUnwrapBugReproduceTest.java | 2 +- 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java index 1fb3d47b3e0..dbc8a90c02e 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java @@ -46,7 +46,7 @@ void populateValueAndReset(int digest, ByteBuf buf) { } @Override - int update(int digest, ByteBuf data, int offset, int len) { + int internalUpdate(int digest, ByteBuf data, int offset, int len) { return Crc32cIntChecksum.resumeChecksum(digest, data, offset, len); } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java index 21be2651a7a..bc1ed499c58 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java @@ -62,7 +62,7 @@ void populateValueAndReset(int digest, ByteBuf buf) { } @Override - int update(int digest, ByteBuf data, int offset, int len) { + int internalUpdate(int digest, ByteBuf data, int offset, int len) { crc.get().update(data, offset, len); return 0; } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 71cef2e3e7b..9fa742c8d13 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -54,7 +54,11 @@ public abstract class DigestManager { abstract int getMacCodeLength(); - abstract int update(int digest, ByteBuf buffer, int offset, int len); + abstract int internalUpdate(int digest, ByteBuf buffer, int offset, int len); + + final int update(int digest, ByteBuf buffer, int offset, int len) { + return internalUpdate(digest, buffer, offset, len); + } abstract void populateValueAndReset(int digest, ByteBuf buffer); diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java index b15499f0cc5..9eaff285662 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java @@ -38,7 +38,7 @@ int getMacCodeLength() { } @Override - int update(int digest, ByteBuf buffer, int offset, int len) { + int internalUpdate(int digest, ByteBuf buffer, int offset, int len) { return 0; } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java index c04c411c6c7..02f357f25a8 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java @@ -96,7 +96,7 @@ void populateValueAndReset(int digest, ByteBuf buffer) { } @Override - int update(int digest, ByteBuf data, int offset, int len) { + int internalUpdate(int digest, ByteBuf data, int offset, int len) { mac.get().update(data.slice(offset, len).nioBuffer()); return 0; } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index fbe3e87074d..9eff3eeaa3a 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -90,7 +90,7 @@ void populateValueAndReset(int digest, ByteBuf buf) { } @Override - int update(int digest, ByteBuf data, int offset, int len) { + int internalUpdate(int digest, ByteBuf data, int offset, int len) { return intHash.resume(digest, data, offset, len); } } From 9a39d6b30169def6a1e222892cccc1962fc46f5f Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 30 Jan 2024 09:18:11 +0200 Subject: [PATCH 05/39] Remove unnecessary unwrapping logic in Java9IntHash --- .../java/com/scurrilous/circe/checksum/Java9IntHash.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index 31af153666e..85c140f3f8b 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -19,7 +19,6 @@ package com.scurrilous.circe.checksum; import io.netty.buffer.ByteBuf; -import io.netty.buffer.CompositeByteBuf; import io.netty.util.concurrent.FastThreadLocal; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -107,13 +106,6 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { } else if (buffer.hasArray()) { int arrayOffset = buffer.arrayOffset() + offset; negCrc = resume(negCrc, buffer.array(), arrayOffset, len); - } else if (buffer instanceof CompositeByteBuf) { - CompositeByteBuf compositeByteBuf = (CompositeByteBuf) buffer; - int loopedCurrent = current; - for (int i = 0; i < compositeByteBuf.numComponents(); i ++) { - loopedCurrent = resume(loopedCurrent, compositeByteBuf.component(i)); - } - return loopedCurrent; } else { byte[] b = TL_BUFFER.get(); int toRead = len; From 986942d11a6b682fbe48871ada6cb02a68c19e6c Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 05:56:52 +0200 Subject: [PATCH 06/39] Add safe way to handle CompositeByteBuf --- .../proto/checksum/DigestManager.java | 73 +++++++++++++++++++ ...ompositeByteBufUnwrapBugReproduceTest.java | 44 +++++++++-- 2 files changed, 110 insertions(+), 7 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 9fa742c8d13..1dd5984c743 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -20,6 +20,7 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.ByteBufUtil; +import io.netty.buffer.DuplicatedByteBuf; import io.netty.buffer.PooledByteBufAllocator; import io.netty.buffer.Unpooled; import io.netty.util.ReferenceCounted; @@ -32,6 +33,7 @@ import org.apache.bookkeeper.proto.BookieProtocol; import org.apache.bookkeeper.proto.DataFormats.LedgerMetadataFormat.DigestType; import org.apache.bookkeeper.util.ByteBufList; +import org.apache.commons.lang3.mutable.MutableInt; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,6 +49,7 @@ public abstract class DigestManager { public static final int METADATA_LENGTH = 32; public static final int LAC_METADATA_LENGTH = 16; + private static final int MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH = 10; final long ledgerId; final boolean useV2Protocol; @@ -57,9 +60,79 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, ByteBuf buffer, int offset, int len); final int update(int digest, ByteBuf buffer, int offset, int len) { + return recursiveSubBufferVisitForDigestUpdate(digest, buffer, offset, len, 0); + } + + private int recursiveSubBufferVisitForDigestUpdate(int digest, ByteBuf buffer, int offset, int len, int depth) { + if (len == 0) { + return digest; + } + if (depth < MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH && !buffer.hasMemoryAddress() && !buffer.hasArray()) { + return visitWrappedBuffersAndCallInternalUpdateForEach(digest, buffer, offset, len, depth); + } return internalUpdate(digest, buffer, offset, len); } + /** + * This method is used to visit the wrapped buffers and call internalUpdate for each of them. + * CompositeByteBuf is one of the wrapped buffer types that will be visited. It can contain multiple + * sub buffers. The sub buffers can be wrapped buffers as well. + * + * Netty doesn't provide an API to visit the wrapped buffers, so we have to use the a hack here. + * + * @param digest current digest value + * @param buffer the buffer to visit + * @param offset the offset in the buffer + * @param len the length in the buffer + * @param depth the recursion depth of the wrapped buffer visit + * @return updated digest value + */ + private int visitWrappedBuffersAndCallInternalUpdateForEach(int digest, ByteBuf buffer, int offset, int len, + int depth) { + // hold the digest in a MutableInt so that it can be updated in the wrapped buffer visit + MutableInt digestRef = new MutableInt(digest); + ByteBuf sliced = buffer.slice(offset, len); + // call getBytes to trigger the wrapped buffer visit + sliced.getBytes(0, new DuplicatedByteBuf(Unpooled.EMPTY_BUFFER) { + @Override + public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { + if (length > 0) { + // recursively visit the sub buffer and update the digest + int updatedDigest = recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), src, srcIndex, + length, depth + 1); + digestRef.setValue(updatedDigest); + } + return this; + } + + @Override + public boolean hasArray() { + // return false so that the wrapped buffer is visited + return false; + } + + @Override + public boolean hasMemoryAddress() { + // return false so that the wrapped buffer is visited + return false; + } + + @Override + public int nioBufferCount() { + // return 0 so that the wrapped buffer is visited + return 0; + } + + @Override + public int capacity() { + // should return sufficient capacity for the total length + return len; + } + }); + // return updated digest value + return digestRef.intValue(); + } + abstract void populateValueAndReset(int digest, ByteBuf buffer); abstract boolean isInt32Digest(); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 9eff3eeaa3a..b80061ab0d2 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -23,14 +23,16 @@ import org.junit.runners.Parameterized; /** - * Reproduces a bug in the checksum calculation when the payload is - * a CompositeByteBuf and this buffer has a reader index state other than 0. + * This test class was added to reproduce a bug in the checksum calculation when + * the payload is a CompositeByteBuf and this buffer has a reader index state other than 0. * The reader index state gets lost in the unwrapping process. * - * There are at least 2 different bugs. One that occurs when the - * payload is >= BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD and the other when - * it's < BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD. - * This test covers both useV2Protocol=true and useV2Protocol=false since the bug is triggered differently. + * There were at least 2 different bugs. One that occured when the + * payload was >= BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD and the other when + * it was < BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD. + * This test covers both useV2Protocol=true and useV2Protocol=false since the bug was triggered differently. + * + * The bug has been fixed and this test is here to make sure it doesn't happen again. */ @RunWith(Parameterized.class) public class CompositeByteBufUnwrapBugReproduceTest { @@ -118,7 +120,14 @@ private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf pay assertArrayEquals(ByteBufUtil.getBytes(payload2.duplicate()), testPayLoad); byte[] output2 = computeDigestAndPackageForSending(intHash, payload2); - assertArrayEquals(output1, output2); + assertArrayEquals(referenceOutput, output2); + + ByteBuf payload3 = wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(payload.retainedDuplicate()); + // this validates that the readable bytes in payload3 match the TEST_PAYLOAD content + assertArrayEquals(ByteBufUtil.getBytes(payload3.duplicate()), testPayLoad); + + byte[] output3 = computeDigestAndPackageForSending(intHash, payload3); + assertArrayEquals(referenceOutput, output3); } private byte[] computeDigestAndPackageForSending(IntHash intHash, ByteBuf data) { @@ -145,6 +154,27 @@ ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload) { return outerComposite; } + ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(ByteBuf payload) { + // create a new buffer with a prefix and the actual payload + ByteBuf prefixedPayload = ByteBufAllocator.DEFAULT.buffer(bufferPrefixLength + payload.readableBytes()); + prefixedPayload.writeBytes(RandomUtils.nextBytes(bufferPrefixLength)); + prefixedPayload.writeBytes(payload); + + CompositeByteBuf innerComposite = ByteBufAllocator.DEFAULT.compositeBuffer(); + innerComposite.addComponent(true, prefixedPayload); + innerComposite.addComponent(true, Unpooled.EMPTY_BUFFER); + + // wrap the buffer in a composite buffer + CompositeByteBuf outerComposite = ByteBufAllocator.DEFAULT.compositeBuffer(); + outerComposite.addComponent(true, innerComposite); + outerComposite.addComponent(true, Unpooled.EMPTY_BUFFER); + + // set reader index state. this is the state that gets lost in the unwrapping process + outerComposite.readerIndex(bufferPrefixLength); + + return outerComposite.duplicate().duplicate(); + } + private static byte[] packagedBufferToBytes(ReferenceCounted packagedBuffer) { byte[] output; if (packagedBuffer instanceof ByteBufList) { From 0669a66bb60d08e6f9742f8e84468be4adb05233 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 06:13:34 +0200 Subject: [PATCH 07/39] Add license header --- ...CompositeByteBufUnwrapBugReproduceTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index b80061ab0d2..a57ad7c80c5 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package org.apache.bookkeeper.proto.checksum; import static org.junit.jupiter.api.Assertions.assertArrayEquals; From 4036a3a0e1e5fe67db0a1765efd6ae2ce18edbe6 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 06:27:37 +0200 Subject: [PATCH 08/39] Fix checkstyle --- .../checksum/CompositeByteBufUnwrapBugReproduceTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index a57ad7c80c5..58c88cd5192 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -19,6 +19,7 @@ package org.apache.bookkeeper.proto.checksum; import static org.junit.jupiter.api.Assertions.assertArrayEquals; + import com.scurrilous.circe.checksum.IntHash; import com.scurrilous.circe.checksum.Java8IntHash; import com.scurrilous.circe.checksum.Java9IntHash; @@ -89,7 +90,8 @@ private static byte[] createTestPayLoad(int payloadSize) { static class TestIntHashDigestManager extends DigestManager { private final IntHash intHash; - public TestIntHashDigestManager(IntHash intHash, long ledgerId, boolean useV2Protocol, ByteBufAllocator allocator) { + public TestIntHashDigestManager(IntHash intHash, long ledgerId, boolean useV2Protocol, + ByteBufAllocator allocator) { super(ledgerId, useV2Protocol, allocator); this.intHash = intHash; } @@ -140,7 +142,8 @@ private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf pay byte[] output2 = computeDigestAndPackageForSending(intHash, payload2); assertArrayEquals(referenceOutput, output2); - ByteBuf payload3 = wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(payload.retainedDuplicate()); + ByteBuf payload3 = wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate( + payload.retainedDuplicate()); // this validates that the readable bytes in payload3 match the TEST_PAYLOAD content assertArrayEquals(ByteBufUtil.getBytes(payload3.duplicate()), testPayLoad); From 7a8c2008609c7a0d54a0a0d663c5f48bdb7bd5d2 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 17:00:36 +0200 Subject: [PATCH 09/39] Refactor ByteBuf visitor solution --- .../proto/checksum/ChildByteBufVisitor.java | 1001 +++++++++++++++++ .../proto/checksum/DigestManager.java | 49 +- 2 files changed, 1010 insertions(+), 40 deletions(-) create mode 100644 bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java new file mode 100644 index 00000000000..ad3f55eeb5c --- /dev/null +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java @@ -0,0 +1,1001 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.bookkeeper.proto.checksum; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufAllocator; +import io.netty.util.ByteProcessor; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.channels.FileChannel; +import java.nio.channels.GatheringByteChannel; +import java.nio.channels.ScatteringByteChannel; +import java.nio.charset.Charset; + +/** + * This class visits the wrapped child buffers of a Netty {@link ByteBuf} for a given offset and length. + *

+ * The Netty ByteBuf API does not provide a method to visit the wrapped child buffers. The + * {@link ByteBuf#unwrap()} method is not suitable for this purpose as it loses the + * {@link ByteBuf#readerIndex()} state, resulting in incorrect offset and length information. For instance, + * unwrapping the slice wrapper of a ByteBuf will lose the offset and the length of the slice. + *

+ * Despite Netty not having a public API for visiting the sub buffers, it is possible to achieve this using + * the {@link ByteBuf#getBytes(int, ByteBuf, int, int)} method. This class uses this method to visit the + * wrapped child buffers by providing a suitable {@link ByteBuf} implementation. This implementation supports + * the role of the destination buffer for the getBytes call. It requires implementing the + * {@link ByteBuf#setBytes(int, ByteBuf, int, int)} method and other methods required by getBytes such as + * {@link ByteBuf#hasArray()}, {@link ByteBuf#hasMemoryAddress()}, {@link ByteBuf#nioBufferCount()} and + * {@link ByteBuf#capacity()}. All other methods in the internal ByteBuf implementation are not supported and will + * throw an exception. This is to ensure correctness and to fail fast if some ByteBuf implementation is not + * following the expected and supported interface contract. + */ +public class ChildByteBufVisitor { + public static void visitChildBuffers(ByteBuf parentBuffer, int parentOffset, int parentLength, + ChildByteBufVisitorCallback callback) { + // call getBytes to trigger the wrapped buffer visit + parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback), 0, parentLength); + } + + public interface ChildByteBufVisitorCallback { + void visitChildBuffer(ByteBuf childBuffer, int childIndex, int childLength); + } + + /** + * A ByteBuf implementation that can be used as the destination buffer for + * a {@link ByteBuf#getBytes(int, ByteBuf)} for visiting the wrapped child buffers. + */ + static class GetBytesCallbackByteBuf extends ByteBuf { + private final ChildByteBufVisitorCallback callback; + private final int capacity; + + GetBytesCallbackByteBuf(ChildByteBufVisitorCallback callback) { + this(callback, Integer.MAX_VALUE); + } + + GetBytesCallbackByteBuf(ChildByteBufVisitorCallback callback, int capacity) { + this.callback = callback; + this.capacity = capacity; + } + + + @Override + public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { + callback.visitChildBuffer(src, srcIndex, length); + return this; + } + + @Override + public boolean hasArray() { + // return false so that the wrapped buffer is visited + return false; + } + + @Override + public boolean hasMemoryAddress() { + // return false so that the wrapped buffer is visited + return false; + } + + @Override + public int nioBufferCount() { + // return 0 so that the wrapped buffer is visited + return 0; + } + + @Override + public int capacity() { + // should return sufficient capacity for the total length + return capacity; + } + + + @Override + public ByteBuf capacity(int newCapacity) { + throw new UnsupportedOperationException(); + } + + @Override + public int maxCapacity() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBufAllocator alloc() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteOrder order() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf order(ByteOrder endianness) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf unwrap() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isDirect() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isReadOnly() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf asReadOnly() { + throw new UnsupportedOperationException(); + } + + @Override + public int readerIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readerIndex(int readerIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public int writerIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writerIndex(int writerIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setIndex(int readerIndex, int writerIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public int readableBytes() { + throw new UnsupportedOperationException(); + } + + @Override + public int writableBytes() { + throw new UnsupportedOperationException(); + } + + @Override + public int maxWritableBytes() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isReadable() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isReadable(int size) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isWritable() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isWritable(int size) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf clear() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf markReaderIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf resetReaderIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf markWriterIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf resetWriterIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf discardReadBytes() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf discardSomeReadBytes() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf ensureWritable(int minWritableBytes) { + throw new UnsupportedOperationException(); + } + + @Override + public int ensureWritable(int minWritableBytes, boolean force) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean getBoolean(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public byte getByte(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public short getUnsignedByte(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public short getShort(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public short getShortLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getUnsignedShort(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getUnsignedShortLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getMedium(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getMediumLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getUnsignedMedium(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getUnsignedMediumLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getInt(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public int getIntLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public long getUnsignedInt(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public long getUnsignedIntLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public long getLong(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public long getLongLE(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public char getChar(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public float getFloat(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public double getDouble(int index) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, ByteBuf dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, ByteBuf dst, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, ByteBuf dst, int dstIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, byte[] dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, byte[] dst, int dstIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, ByteBuffer dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf getBytes(int index, OutputStream out, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int getBytes(int index, GatheringByteChannel out, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int getBytes(int index, FileChannel out, long position, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public CharSequence getCharSequence(int index, int length, Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBoolean(int index, boolean value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setByte(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setShort(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setShortLE(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setMedium(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setMediumLE(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setInt(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setIntLE(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setLong(int index, long value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setLongLE(int index, long value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setChar(int index, int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setFloat(int index, float value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setDouble(int index, double value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBytes(int index, ByteBuf src) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBytes(int index, ByteBuf src, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBytes(int index, byte[] src) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBytes(int index, byte[] src, int srcIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setBytes(int index, ByteBuffer src) { + throw new UnsupportedOperationException(); + } + + @Override + public int setBytes(int index, InputStream in, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int setBytes(int index, ScatteringByteChannel in, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int setBytes(int index, FileChannel in, long position, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf setZero(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public int setCharSequence(int index, CharSequence sequence, Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean readBoolean() { + throw new UnsupportedOperationException(); + } + + @Override + public byte readByte() { + throw new UnsupportedOperationException(); + } + + @Override + public short readUnsignedByte() { + throw new UnsupportedOperationException(); + } + + @Override + public short readShort() { + throw new UnsupportedOperationException(); + } + + @Override + public short readShortLE() { + throw new UnsupportedOperationException(); + } + + @Override + public int readUnsignedShort() { + throw new UnsupportedOperationException(); + } + + @Override + public int readUnsignedShortLE() { + throw new UnsupportedOperationException(); + } + + @Override + public int readMedium() { + throw new UnsupportedOperationException(); + } + + @Override + public int readMediumLE() { + throw new UnsupportedOperationException(); + } + + @Override + public int readUnsignedMedium() { + throw new UnsupportedOperationException(); + } + + @Override + public int readUnsignedMediumLE() { + throw new UnsupportedOperationException(); + } + + @Override + public int readInt() { + throw new UnsupportedOperationException(); + } + + @Override + public int readIntLE() { + throw new UnsupportedOperationException(); + } + + @Override + public long readUnsignedInt() { + throw new UnsupportedOperationException(); + } + + @Override + public long readUnsignedIntLE() { + throw new UnsupportedOperationException(); + } + + @Override + public long readLong() { + throw new UnsupportedOperationException(); + } + + @Override + public long readLongLE() { + throw new UnsupportedOperationException(); + } + + @Override + public char readChar() { + throw new UnsupportedOperationException(); + } + + @Override + public float readFloat() { + throw new UnsupportedOperationException(); + } + + @Override + public double readDouble() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readSlice(int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readRetainedSlice(int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(ByteBuf dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(ByteBuf dst, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(ByteBuf dst, int dstIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(byte[] dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(byte[] dst, int dstIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(ByteBuffer dst) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf readBytes(OutputStream out, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int readBytes(GatheringByteChannel out, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public CharSequence readCharSequence(int length, Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public int readBytes(FileChannel out, long position, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf skipBytes(int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBoolean(boolean value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeByte(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeShort(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeShortLE(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeMedium(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeMediumLE(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeInt(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeIntLE(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeLong(long value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeLongLE(long value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeChar(int value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeFloat(float value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeDouble(double value) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(ByteBuf src) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(ByteBuf src, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(ByteBuf src, int srcIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(byte[] src) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(byte[] src, int srcIndex, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeBytes(ByteBuffer src) { + throw new UnsupportedOperationException(); + } + + @Override + public int writeBytes(InputStream in, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int writeBytes(ScatteringByteChannel in, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int writeBytes(FileChannel in, long position, int length) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf writeZero(int length) { + throw new UnsupportedOperationException(); + } + + @Override + public int writeCharSequence(CharSequence sequence, Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public int indexOf(int fromIndex, int toIndex, byte value) { + throw new UnsupportedOperationException(); + } + + @Override + public int bytesBefore(byte value) { + throw new UnsupportedOperationException(); + } + + @Override + public int bytesBefore(int length, byte value) { + throw new UnsupportedOperationException(); + } + + @Override + public int bytesBefore(int index, int length, byte value) { + throw new UnsupportedOperationException(); + } + + @Override + public int forEachByte(ByteProcessor processor) { + throw new UnsupportedOperationException(); + } + + @Override + public int forEachByte(int index, int length, ByteProcessor processor) { + throw new UnsupportedOperationException(); + } + + @Override + public int forEachByteDesc(ByteProcessor processor) { + throw new UnsupportedOperationException(); + } + + @Override + public int forEachByteDesc(int index, int length, ByteProcessor processor) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf copy() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf copy(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf slice() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf retainedSlice() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf slice(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf retainedSlice(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf duplicate() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf retainedDuplicate() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuffer nioBuffer() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuffer nioBuffer(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuffer internalNioBuffer(int index, int length) { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuffer[] nioBuffers() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuffer[] nioBuffers(int index, int length) { + throw new UnsupportedOperationException(); + } + + + @Override + public byte[] array() { + throw new UnsupportedOperationException(); + } + + @Override + public int arrayOffset() { + throw new UnsupportedOperationException(); + } + @Override + public long memoryAddress() { + throw new UnsupportedOperationException(); + } + + @Override + public String toString(Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public String toString(int index, int length, Charset charset) { + throw new UnsupportedOperationException(); + } + + @Override + public int hashCode() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean equals(Object obj) { + throw new UnsupportedOperationException(); + } + + @Override + public int compareTo(ByteBuf buffer) { + throw new UnsupportedOperationException(); + } + + @Override + public String toString() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf retain(int increment) { + throw new UnsupportedOperationException(); + } + + @Override + public int refCnt() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf retain() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf touch() { + throw new UnsupportedOperationException(); + } + + @Override + public ByteBuf touch(Object hint) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean release() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean release(int decrement) { + throw new UnsupportedOperationException(); + } + } +} diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 1dd5984c743..b09cc5818db 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -20,7 +20,6 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.ByteBufUtil; -import io.netty.buffer.DuplicatedByteBuf; import io.netty.buffer.PooledByteBufAllocator; import io.netty.buffer.Unpooled; import io.netty.util.ReferenceCounted; @@ -68,7 +67,7 @@ private int recursiveSubBufferVisitForDigestUpdate(int digest, ByteBuf buffer, i return digest; } if (depth < MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH && !buffer.hasMemoryAddress() && !buffer.hasArray()) { - return visitWrappedBuffersAndCallInternalUpdateForEach(digest, buffer, offset, len, depth); + return visitChildBuffersAndCallInternalUpdateForEach(digest, buffer, offset, len, depth); } return internalUpdate(digest, buffer, offset, len); } @@ -87,46 +86,16 @@ private int recursiveSubBufferVisitForDigestUpdate(int digest, ByteBuf buffer, i * @param depth the recursion depth of the wrapped buffer visit * @return updated digest value */ - private int visitWrappedBuffersAndCallInternalUpdateForEach(int digest, ByteBuf buffer, int offset, int len, - int depth) { + private int visitChildBuffersAndCallInternalUpdateForEach(int digest, ByteBuf buffer, int offset, int len, + int depth) { // hold the digest in a MutableInt so that it can be updated in the wrapped buffer visit MutableInt digestRef = new MutableInt(digest); - ByteBuf sliced = buffer.slice(offset, len); - // call getBytes to trigger the wrapped buffer visit - sliced.getBytes(0, new DuplicatedByteBuf(Unpooled.EMPTY_BUFFER) { - @Override - public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { - if (length > 0) { - // recursively visit the sub buffer and update the digest - int updatedDigest = recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), src, srcIndex, - length, depth + 1); - digestRef.setValue(updatedDigest); - } - return this; - } - - @Override - public boolean hasArray() { - // return false so that the wrapped buffer is visited - return false; - } - - @Override - public boolean hasMemoryAddress() { - // return false so that the wrapped buffer is visited - return false; - } - - @Override - public int nioBufferCount() { - // return 0 so that the wrapped buffer is visited - return 0; - } - - @Override - public int capacity() { - // should return sufficient capacity for the total length - return len; + ChildByteBufVisitor.visitChildBuffers(buffer, offset, len, (ByteBuf childBuffer, int srcIndex, int length) -> { + if (length > 0) { + // recursively visit the sub buffer and update the digest + int updatedDigest = + recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, srcIndex, length, depth + 1); + digestRef.setValue(updatedDigest); } }); // return updated digest value From b5b42eed812ee04ccdf67ad86a520dd6b790a694 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 17:07:22 +0200 Subject: [PATCH 10/39] Fix checkstyle --- .../org/apache/bookkeeper/proto/checksum/DigestManager.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index b09cc5818db..428db4fb1d3 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -94,7 +94,8 @@ private int visitChildBuffersAndCallInternalUpdateForEach(int digest, ByteBuf bu if (length > 0) { // recursively visit the sub buffer and update the digest int updatedDigest = - recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, srcIndex, length, depth + 1); + recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, srcIndex, length, + depth + 1); digestRef.setValue(updatedDigest); } }); From 81e9d2df38ff15e4fada3969ff866547c83f27eb Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 17:09:18 +0200 Subject: [PATCH 11/39] Reformat --- .../proto/checksum/DigestManager.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 428db4fb1d3..ec153b9173e 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -90,15 +90,16 @@ private int visitChildBuffersAndCallInternalUpdateForEach(int digest, ByteBuf bu int depth) { // hold the digest in a MutableInt so that it can be updated in the wrapped buffer visit MutableInt digestRef = new MutableInt(digest); - ChildByteBufVisitor.visitChildBuffers(buffer, offset, len, (ByteBuf childBuffer, int srcIndex, int length) -> { - if (length > 0) { - // recursively visit the sub buffer and update the digest - int updatedDigest = - recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, srcIndex, length, - depth + 1); - digestRef.setValue(updatedDigest); - } - }); + ChildByteBufVisitor.visitChildBuffers(buffer, offset, len, + (ByteBuf childBuffer, int childIndex, int childLength) -> { + if (childLength > 0) { + // recursively visit the sub buffer and update the digest + int updatedDigest = + recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, childIndex, + childLength, depth + 1); + digestRef.setValue(updatedDigest); + } + }); // return updated digest value return digestRef.intValue(); } From 6c7e6e001433424964bba58f8c6da3fc5aa90250 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 18:14:41 +0200 Subject: [PATCH 12/39] Refactor recursive visiting --- ...yteBufVisitor.java => ByteBufVisitor.java} | 80 ++++++++++++++++--- .../proto/checksum/DigestManager.java | 38 +-------- 2 files changed, 70 insertions(+), 48 deletions(-) rename bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/{ChildByteBufVisitor.java => ByteBufVisitor.java} (88%) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java similarity index 88% rename from bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java rename to bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java index ad3f55eeb5c..333aeaf877a 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ChildByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java @@ -31,7 +31,7 @@ import java.nio.charset.Charset; /** - * This class visits the wrapped child buffers of a Netty {@link ByteBuf} for a given offset and length. + * This class visits the possible wrapped child buffers of a Netty {@link ByteBuf} for a given offset and length. *

* The Netty ByteBuf API does not provide a method to visit the wrapped child buffers. The * {@link ByteBuf#unwrap()} method is not suitable for this purpose as it loses the @@ -48,15 +48,71 @@ * throw an exception. This is to ensure correctness and to fail fast if some ByteBuf implementation is not * following the expected and supported interface contract. */ -public class ChildByteBufVisitor { - public static void visitChildBuffers(ByteBuf parentBuffer, int parentOffset, int parentLength, - ChildByteBufVisitorCallback callback) { - // call getBytes to trigger the wrapped buffer visit - parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback), 0, parentLength); +public class ByteBufVisitor { + private static final int DEFAULT_VISIT_MAX_DEPTH = 10; + + /** + * This method traverses the potential nested composite buffers of the provided buffer, given a specific offset and + * length. The traversal continues until it encounters a buffer that is backed by an array or a memory address, + * which allows for the inspection of individual buffer segments without the need for data duplication. + * If no such wrapped buffer is found, the callback function is invoked with the original buffer, offset, + * and length as parameters. + * + * @param buffer the buffer to visit + * @param offset the offset for the buffer + * @param length the length for the buffer + * @param callback the callback to call for each visited buffer + */ + public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback) { + visitBuffers(buffer, offset, length, callback, DEFAULT_VISIT_MAX_DEPTH); + } + + /** + * The callback interface for visiting buffers. + */ + public interface ByteBufVisitorCallback { + void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength); } - public interface ChildByteBufVisitorCallback { - void visitChildBuffer(ByteBuf childBuffer, int childIndex, int childLength); + /** + * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback)}. This method allows to specify + * the maximum depth of recursion for visiting wrapped buffers. + */ + public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, + int maxDepth) { + doRecursivelyVisitBuffers(buffer, offset, length, callback, maxDepth, 0); + } + + private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, int length, + ByteBufVisitorCallback callback, int maxDepth, int depth) { + if (length == 0) { + // skip visiting empty buffers + return; + } + // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address + // and the max depth has not been reached + if (depth < maxDepth && !buffer.hasMemoryAddress() && !buffer.hasArray()) { + visitBuffersImpl(buffer, offset, length, (visitBuffer, visitIndex, visitLength) -> { + if (visitBuffer == buffer && visitIndex == offset && visitLength == length) { + // visit the buffer since it was already passed to visitBuffersImpl and further recursion + // would cause unnecessary recursion up to the max depth of recursion + callback.visitBuffer(visitBuffer, visitIndex, visitLength); + } else { + // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively + doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, callback, maxDepth, depth + 1); + } + }); + } else { + // visit the buffer + callback.visitBuffer(buffer, offset, length); + } + } + + // Implementation for visiting a single buffer level using {@link ByteBuf#getBytes(int, ByteBuf, int, int)} + private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, int parentLength, + ByteBufVisitorCallback callback) { + // call getBytes to trigger the wrapped buffer visit + parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback), 0, parentLength); } /** @@ -64,14 +120,14 @@ public interface ChildByteBufVisitorCallback { * a {@link ByteBuf#getBytes(int, ByteBuf)} for visiting the wrapped child buffers. */ static class GetBytesCallbackByteBuf extends ByteBuf { - private final ChildByteBufVisitorCallback callback; + private final ByteBufVisitorCallback callback; private final int capacity; - GetBytesCallbackByteBuf(ChildByteBufVisitorCallback callback) { + GetBytesCallbackByteBuf(ByteBufVisitorCallback callback) { this(callback, Integer.MAX_VALUE); } - GetBytesCallbackByteBuf(ChildByteBufVisitorCallback callback, int capacity) { + GetBytesCallbackByteBuf(ByteBufVisitorCallback callback, int capacity) { this.callback = callback; this.capacity = capacity; } @@ -79,7 +135,7 @@ static class GetBytesCallbackByteBuf extends ByteBuf { @Override public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { - callback.visitChildBuffer(src, srcIndex, length); + callback.visitBuffer(src, srcIndex, length); return this; } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index ec153b9173e..865a9700a39 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -48,7 +48,6 @@ public abstract class DigestManager { public static final int METADATA_LENGTH = 32; public static final int LAC_METADATA_LENGTH = 16; - private static final int MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH = 10; final long ledgerId; final boolean useV2Protocol; @@ -59,48 +58,15 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, ByteBuf buffer, int offset, int len); final int update(int digest, ByteBuf buffer, int offset, int len) { - return recursiveSubBufferVisitForDigestUpdate(digest, buffer, offset, len, 0); - } - - private int recursiveSubBufferVisitForDigestUpdate(int digest, ByteBuf buffer, int offset, int len, int depth) { - if (len == 0) { - return digest; - } - if (depth < MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH && !buffer.hasMemoryAddress() && !buffer.hasArray()) { - return visitChildBuffersAndCallInternalUpdateForEach(digest, buffer, offset, len, depth); - } - return internalUpdate(digest, buffer, offset, len); - } - - /** - * This method is used to visit the wrapped buffers and call internalUpdate for each of them. - * CompositeByteBuf is one of the wrapped buffer types that will be visited. It can contain multiple - * sub buffers. The sub buffers can be wrapped buffers as well. - * - * Netty doesn't provide an API to visit the wrapped buffers, so we have to use the a hack here. - * - * @param digest current digest value - * @param buffer the buffer to visit - * @param offset the offset in the buffer - * @param len the length in the buffer - * @param depth the recursion depth of the wrapped buffer visit - * @return updated digest value - */ - private int visitChildBuffersAndCallInternalUpdateForEach(int digest, ByteBuf buffer, int offset, int len, - int depth) { - // hold the digest in a MutableInt so that it can be updated in the wrapped buffer visit MutableInt digestRef = new MutableInt(digest); - ChildByteBufVisitor.visitChildBuffers(buffer, offset, len, + ByteBufVisitor.visitBuffers(buffer, offset, len, (ByteBuf childBuffer, int childIndex, int childLength) -> { if (childLength > 0) { // recursively visit the sub buffer and update the digest - int updatedDigest = - recursiveSubBufferVisitForDigestUpdate(digestRef.intValue(), childBuffer, childIndex, - childLength, depth + 1); + int updatedDigest = internalUpdate(digestRef.intValue(), childBuffer, childIndex, childLength); digestRef.setValue(updatedDigest); } }); - // return updated digest value return digestRef.intValue(); } From c19ae5d6bf66c56dfae7401de9dc54ccd117a79f Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 31 Jan 2024 19:29:48 +0200 Subject: [PATCH 13/39] Revisit equals, hashCode and toString --- .../proto/checksum/ByteBufVisitor.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java index 333aeaf877a..629fe42ff33 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java @@ -1000,58 +1000,58 @@ public String toString(int index, int length, Charset charset) { } @Override - public int hashCode() { + public int compareTo(ByteBuf buffer) { throw new UnsupportedOperationException(); } @Override - public boolean equals(Object obj) { + public ByteBuf retain(int increment) { throw new UnsupportedOperationException(); } @Override - public int compareTo(ByteBuf buffer) { + public int refCnt() { throw new UnsupportedOperationException(); } @Override - public String toString() { + public ByteBuf retain() { throw new UnsupportedOperationException(); } @Override - public ByteBuf retain(int increment) { + public ByteBuf touch() { throw new UnsupportedOperationException(); } @Override - public int refCnt() { + public ByteBuf touch(Object hint) { throw new UnsupportedOperationException(); } @Override - public ByteBuf retain() { + public boolean release() { throw new UnsupportedOperationException(); } @Override - public ByteBuf touch() { + public boolean release(int decrement) { throw new UnsupportedOperationException(); } @Override - public ByteBuf touch(Object hint) { - throw new UnsupportedOperationException(); + public String toString() { + return getClass().getSimpleName() + '@' + Integer.toHexString(System.identityHashCode(this)); } @Override - public boolean release() { - throw new UnsupportedOperationException(); + public int hashCode() { + return System.identityHashCode(this); } @Override - public boolean release(int decrement) { - throw new UnsupportedOperationException(); + public boolean equals(Object obj) { + return obj == this; } } } From 8928f98de0a87726f6f9292fa28d789d1ee2b54f Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 07:32:50 +0200 Subject: [PATCH 14/39] Refactor test case --- ...ompositeByteBufUnwrapBugReproduceTest.java | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 58c88cd5192..142ecc494d6 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -130,25 +130,29 @@ public void shouldCalculateChecksumForCompositeBuffer() { } private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf payload, byte[] referenceOutput) { - byte[] output1 = computeDigestAndPackageForSending(intHash, payload.retainedDuplicate()); - assertArrayEquals(referenceOutput, output1); + assertDigestAndPackageScenario(intHash, payload.retainedDuplicate(), referenceOutput, + "plain payload, no wrapping"); - // wrap the payload in a CompositeByteBuf which contains another buffer where the payload contains a prefix - // the reader index is set past the prefix ByteBuf payload2 = wrapWithPrefixAndCompositeByteBufWithReaderIndexState(payload.retainedDuplicate()); - // this validates that the readable bytes in payload2 match the TEST_PAYLOAD content - assertArrayEquals(ByteBufUtil.getBytes(payload2.duplicate()), testPayLoad); - - byte[] output2 = computeDigestAndPackageForSending(intHash, payload2); - assertArrayEquals(referenceOutput, output2); + assertDigestAndPackageScenario(intHash, payload2, referenceOutput, + "payload with prefix wrapped in CompositeByteBuf with readerIndex state"); ByteBuf payload3 = wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate( payload.retainedDuplicate()); - // this validates that the readable bytes in payload3 match the TEST_PAYLOAD content - assertArrayEquals(ByteBufUtil.getBytes(payload3.duplicate()), testPayLoad); + assertDigestAndPackageScenario(intHash, payload3, referenceOutput, + "payload with prefix wrapped in 2 layers of CompositeByteBuf with readerIndex state in the outer " + + "composite. In addition, the outer composite is duplicated twice."); + } - byte[] output3 = computeDigestAndPackageForSending(intHash, payload3); - assertArrayEquals(referenceOutput, output3); + private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, + String scenario) { + // this validates that the readable bytes in the payload match the TEST_PAYLOAD content + assertArrayEquals(ByteBufUtil.getBytes(payload.duplicate()), testPayLoad, + "input is invalid for scenario '" + scenario + "'"); + // compute the digest and package + byte[] output = computeDigestAndPackageForSending(intHash, payload); + // this validates that the output matches the reference output + assertArrayEquals(referenceOutput, output, "output is invalid for scenario '" + scenario + "'"); } private byte[] computeDigestAndPackageForSending(IntHash intHash, ByteBuf data) { From 19e33cad79f925a6401a2d493de99168c32f4489 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 07:44:23 +0200 Subject: [PATCH 15/39] Add support for UnpooledHeapByteBuf.getBytes which passes an array --- .../bookkeeper/proto/checksum/ByteBufVisitor.java | 12 +++++++----- .../CompositeByteBufUnwrapBugReproduceTest.java | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java index 629fe42ff33..098229bd9bb 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java @@ -19,6 +19,7 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; +import io.netty.buffer.Unpooled; import io.netty.util.ByteProcessor; import java.io.IOException; import java.io.InputStream; @@ -139,6 +140,12 @@ public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { return this; } + @Override + public ByteBuf setBytes(int index, byte[] src, int srcIndex, int length) { + callback.visitBuffer(Unpooled.wrappedBuffer(src), srcIndex, length); + return this; + } + @Override public boolean hasArray() { // return false so that the wrapped buffer is visited @@ -544,11 +551,6 @@ public ByteBuf setBytes(int index, byte[] src) { throw new UnsupportedOperationException(); } - @Override - public ByteBuf setBytes(int index, byte[] src, int srcIndex, int length) { - throw new UnsupportedOperationException(); - } - @Override public ByteBuf setBytes(int index, ByteBuffer src) { throw new UnsupportedOperationException(); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 142ecc494d6..4f7c9ec7666 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -142,6 +142,10 @@ private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf pay assertDigestAndPackageScenario(intHash, payload3, referenceOutput, "payload with prefix wrapped in 2 layers of CompositeByteBuf with readerIndex state in the outer " + "composite. In addition, the outer composite is duplicated twice."); + + ByteBuf payload4 = wrapInCompositeByteBufAndSlice(payload.retainedDuplicate()); + assertDigestAndPackageScenario(intHash, payload4, referenceOutput, + "payload with prefix wrapped in CompositeByteBuf and sliced"); } private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, @@ -200,6 +204,16 @@ ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultiple return outerComposite.duplicate().duplicate(); } + ByteBuf wrapInCompositeByteBufAndSlice(ByteBuf payload) { + // create a composite buffer + CompositeByteBuf compositeWithPrefix = ByteBufAllocator.DEFAULT.compositeBuffer(); + compositeWithPrefix.addComponent(true, Unpooled.wrappedBuffer(RandomUtils.nextBytes(bufferPrefixLength))); + compositeWithPrefix.addComponent(true, payload); + + // return a slice of the composite buffer so that it returns the payload + return compositeWithPrefix.slice(bufferPrefixLength, payload.readableBytes()); + } + private static byte[] packagedBufferToBytes(ReferenceCounted packagedBuffer) { byte[] output; if (packagedBuffer instanceof ByteBufList) { From 86d002d558ba896e815e510d34baba92077fcf2a Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 08:23:52 +0200 Subject: [PATCH 16/39] Add support for visiting buffers backed by byte[] arrays - getBytes calls setBytes with a byte[] argument for heap ByteBufs --- .../proto/checksum/ByteBufVisitor.java | 26 +++++++++++++------ .../proto/checksum/CRC32CDigestManager.java | 5 ++++ .../proto/checksum/CRC32DigestManager.java | 7 +++++ .../proto/checksum/DigestManager.java | 26 +++++++++++++++---- .../checksum/DirectMemoryCRC32Digest.java | 18 +++++++++++-- .../proto/checksum/DummyDigestManager.java | 5 ++++ .../proto/checksum/MacDigestManager.java | 6 +++++ .../proto/checksum/StandardCRC32Digest.java | 5 ++++ ...ompositeByteBufUnwrapBugReproduceTest.java | 5 ++++ .../circe/checksum/Crc32cIntChecksum.java | 21 ++++++++++++--- .../scurrilous/circe/checksum/IntHash.java | 2 ++ .../circe/checksum/Java8IntHash.java | 5 ++++ .../circe/checksum/Java9IntHash.java | 3 ++- .../scurrilous/circe/checksum/JniIntHash.java | 5 ++++ 14 files changed, 120 insertions(+), 19 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java index 098229bd9bb..28ce3067357 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java @@ -73,6 +73,7 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufV */ public interface ByteBufVisitorCallback { void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength); + void visitArray(byte[] visitArray, int visitIndex, int visitLength); } /** @@ -93,14 +94,23 @@ private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, int le // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address // and the max depth has not been reached if (depth < maxDepth && !buffer.hasMemoryAddress() && !buffer.hasArray()) { - visitBuffersImpl(buffer, offset, length, (visitBuffer, visitIndex, visitLength) -> { - if (visitBuffer == buffer && visitIndex == offset && visitLength == length) { - // visit the buffer since it was already passed to visitBuffersImpl and further recursion - // would cause unnecessary recursion up to the max depth of recursion - callback.visitBuffer(visitBuffer, visitIndex, visitLength); - } else { - // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively - doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, callback, maxDepth, depth + 1); + visitBuffersImpl(buffer, offset, length, new ByteBufVisitorCallback() { + @Override + public void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitBuffer == buffer && visitIndex == offset && visitLength == length) { + // visit the buffer since it was already passed to visitBuffersImpl and further recursion + // would cause unnecessary recursion up to the max depth of recursion + callback.visitBuffer(visitBuffer, visitIndex, visitLength); + } else { + // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively + doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, callback, maxDepth, depth + 1); + } + } + + @Override + public void visitArray(byte[] visitArray, int visitIndex, int visitLength) { + // visit the array + callback.visitArray(visitArray, visitIndex, visitLength); } }); } else { diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java index dbc8a90c02e..0b9c469beb1 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java @@ -49,4 +49,9 @@ void populateValueAndReset(int digest, ByteBuf buf) { int internalUpdate(int digest, ByteBuf data, int offset, int len) { return Crc32cIntChecksum.resumeChecksum(digest, data, offset, len); } + + @Override + int internalUpdate(int digest, byte[] buffer, int offset, int len) { + return Crc32cIntChecksum.resumeChecksum(digest, buffer, offset, len); + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java index bc1ed499c58..12d21836c9d 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java @@ -34,6 +34,7 @@ interface CRC32Digest { long getValueAndReset(); void update(ByteBuf buf, int offset, int len); + void update(byte[] buffer, int offset, int len); } private static final FastThreadLocal crc = new FastThreadLocal() { @@ -67,6 +68,12 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { return 0; } + @Override + int internalUpdate(int digest, byte[] buffer, int offset, int len) { + crc.get().update(buffer, offset, len); + return 0; + } + @Override boolean isInt32Digest() { // This is stored as 8 bytes diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 865a9700a39..12156bcf12e 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -57,14 +57,30 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, ByteBuf buffer, int offset, int len); + abstract int internalUpdate(int digest, byte[] buffer, int offset, int len); + final int update(int digest, ByteBuf buffer, int offset, int len) { MutableInt digestRef = new MutableInt(digest); ByteBufVisitor.visitBuffers(buffer, offset, len, - (ByteBuf childBuffer, int childIndex, int childLength) -> { - if (childLength > 0) { - // recursively visit the sub buffer and update the digest - int updatedDigest = internalUpdate(digestRef.intValue(), childBuffer, childIndex, childLength); - digestRef.setValue(updatedDigest); + new ByteBufVisitor.ByteBufVisitorCallback() { + @Override + public void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitLength > 0) { + // recursively visit the sub buffer and update the digest + int updatedDigest = + internalUpdate(digestRef.intValue(), visitBuffer, visitIndex, visitLength); + digestRef.setValue(updatedDigest); + } + } + + @Override + public void visitArray(byte[] visitArray, int visitIndex, int visitLength) { + if (visitLength > 0) { + // update the digest with the array + int updatedDigest = + internalUpdate(digestRef.intValue(), visitArray, visitIndex, visitLength); + digestRef.setValue(updatedDigest); + } } }); return digestRef.intValue(); diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java index eda223ef7f3..f0814a72130 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java @@ -50,18 +50,32 @@ public void update(ByteBuf buf, int index, int length) { crcValue = (int) updateByteBuffer.invoke(null, crcValue, buf.memoryAddress(), index, length); } else if (buf.hasArray()) { // Use the internal method to update from array based - crcValue = (int) updateBytes.invoke(null, crcValue, buf.array(), buf.arrayOffset() + index, length); + crcValue = updateArray(crcValue, buf.array(), buf.arrayOffset() + index, length); } else { // Fallback to data copy if buffer is not contiguous byte[] b = new byte[length]; buf.getBytes(index, b, 0, length); - crcValue = (int) updateBytes.invoke(null, crcValue, b, 0, b.length); + crcValue = updateArray(crcValue, b, 0, length); } } catch (IllegalAccessException | InvocationTargetException e) { throw new RuntimeException(e); } } + private static int updateArray(int crcValue, byte[] buf, int offset, int length) + throws IllegalAccessException, InvocationTargetException { + return (int) updateBytes.invoke(null, crcValue, buf, offset, length); + } + + @Override + public void update(byte[] buffer, int offset, int len) { + try { + crcValue = (int) updateBytes.invoke(null, crcValue, buffer, offset, len); + } catch (IllegalAccessException | InvocationTargetException e) { + throw new RuntimeException(e); + } + } + private static final Method updateByteBuffer; private static final Method updateBytes; diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java index 9eaff285662..fd1f4b83c00 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java @@ -42,6 +42,11 @@ int internalUpdate(int digest, ByteBuf buffer, int offset, int len) { return 0; } + @Override + int internalUpdate(int digest, byte[] buffer, int offset, int len) { + return 0; + } + @Override void populateValueAndReset(int digest, ByteBuf buffer) {} diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java index 02f357f25a8..8ec5256bdc2 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java @@ -101,6 +101,12 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { return 0; } + @Override + int internalUpdate(int digest, byte[] buffer, int offset, int len) { + mac.get().update(buffer, offset, len); + return 0; + } + @Override boolean isInt32Digest() { return false; diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/StandardCRC32Digest.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/StandardCRC32Digest.java index 3d48f0ef7da..7635e3e9f20 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/StandardCRC32Digest.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/StandardCRC32Digest.java @@ -39,4 +39,9 @@ public long getValueAndReset() { public void update(ByteBuf buf, int offset, int len) { crc.update(buf.slice(offset, len).nioBuffer()); } + + @Override + public void update(byte[] buffer, int offset, int len) { + crc.update(buffer, offset, len); + } } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 4f7c9ec7666..ba670253af7 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -115,6 +115,11 @@ void populateValueAndReset(int digest, ByteBuf buf) { int internalUpdate(int digest, ByteBuf data, int offset, int len) { return intHash.resume(digest, data, offset, len); } + + @Override + int internalUpdate(int digest, byte[] buffer, int offset, int len) { + return intHash.resume(digest, buffer, offset, len); + } } @Test diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java index 65a77b1492b..0f0bcbe4528 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java @@ -71,12 +71,27 @@ public static int resumeChecksum(int previousChecksum, ByteBuf payload) { /** * Computes incremental checksum with input previousChecksum and input payload * - * @param previousChecksum : previously computed checksum - * @param payload - * @return + * @param previousChecksum the previously computed checksum + * @param payload the data for which the checksum is to be computed + * @param offset the starting position in the payload + * @param len the number of bytes to include in the checksum computation + * @return the updated checksum */ public static int resumeChecksum(int previousChecksum, ByteBuf payload, int offset, int len) { return CRC32C_HASH.resume(previousChecksum, payload, offset, len); } + /** + * Computes incremental checksum with input previousChecksum and input payload + * + * @param previousChecksum the previously computed checksum + * @param payload the data for which the checksum is to be computed + * @param offset the starting position in the payload + * @param len the number of bytes to include in the checksum computation + * @return the updated checksum + */ + public static int resumeChecksum(int previousChecksum, byte[] payload, int offset, int len) { + return CRC32C_HASH.resume(previousChecksum, payload, offset, len); + } + } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java index e8922e3a16b..84fb1c73364 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java @@ -28,4 +28,6 @@ public interface IntHash { int resume(int current, ByteBuf buffer); int resume(int current, ByteBuf buffer, int offset, int len); + + int resume(int current, byte[] buffer, int offset, int len); } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java index fd548bc4de4..bbf7d5aa6e8 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java @@ -53,4 +53,9 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { return hash.resume(current, buffer.slice(offset, len).nioBuffer()); } } + + @Override + public int resume(int current, byte[] buffer, int offset, int len) { + return hash.resume(current, buffer, offset, len); + } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index 85c140f3f8b..dc2ec58ef79 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -84,7 +84,8 @@ private int resume(int current, long address, int offset, int length) { } } - private int resume(int current, byte[] array, int offset, int length) { + @Override + public int resume(int current, byte[] array, int offset, int length) { try { return (int) UPDATE_BYTES.invoke(null, current, array, offset, offset + length); } catch (IllegalAccessException | InvocationTargetException e) { diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java index e8e87bf6b1a..7c125cca021 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java @@ -51,4 +51,9 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { return hash.resume(current, buffer.slice(offset, len).nioBuffer()); } } + + @Override + public int resume(int current, byte[] buffer, int offset, int len) { + return hash.resume(current, buffer, offset, len); + } } From e793872e3e12d04b8d15ceb8ffac6508d6c3f8dc Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 08:54:25 +0200 Subject: [PATCH 17/39] Move ByteBufVisitor to org.apache.bookkeeper.util package --- .../org/apache/bookkeeper/proto/checksum/DigestManager.java | 1 + .../bookkeeper/{proto/checksum => util}/ByteBufVisitor.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) rename bookkeeper-server/src/main/java/org/apache/bookkeeper/{proto/checksum => util}/ByteBufVisitor.java (99%) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 12156bcf12e..01d4d0ed11b 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -32,6 +32,7 @@ import org.apache.bookkeeper.proto.BookieProtocol; import org.apache.bookkeeper.proto.DataFormats.LedgerMetadataFormat.DigestType; import org.apache.bookkeeper.util.ByteBufList; +import org.apache.bookkeeper.util.ByteBufVisitor; import org.apache.commons.lang3.mutable.MutableInt; import org.slf4j.Logger; import org.slf4j.LoggerFactory; diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java similarity index 99% rename from bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java rename to bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 28ce3067357..c35df1c8c2f 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -15,7 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.bookkeeper.proto.checksum; +package org.apache.bookkeeper.util; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; From 6508bf5a26069352702f695d54e9b60b3681c0cc Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 08:58:47 +0200 Subject: [PATCH 18/39] Update javadoc --- .../apache/bookkeeper/util/ByteBufVisitor.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index c35df1c8c2f..cdeda13534b 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -36,18 +36,18 @@ *

* The Netty ByteBuf API does not provide a method to visit the wrapped child buffers. The * {@link ByteBuf#unwrap()} method is not suitable for this purpose as it loses the - * {@link ByteBuf#readerIndex()} state, resulting in incorrect offset and length information. For instance, - * unwrapping the slice wrapper of a ByteBuf will lose the offset and the length of the slice. + * {@link ByteBuf#readerIndex()} state, resulting in incorrect offset and length information. *

* Despite Netty not having a public API for visiting the sub buffers, it is possible to achieve this using * the {@link ByteBuf#getBytes(int, ByteBuf, int, int)} method. This class uses this method to visit the * wrapped child buffers by providing a suitable {@link ByteBuf} implementation. This implementation supports * the role of the destination buffer for the getBytes call. It requires implementing the - * {@link ByteBuf#setBytes(int, ByteBuf, int, int)} method and other methods required by getBytes such as - * {@link ByteBuf#hasArray()}, {@link ByteBuf#hasMemoryAddress()}, {@link ByteBuf#nioBufferCount()} and - * {@link ByteBuf#capacity()}. All other methods in the internal ByteBuf implementation are not supported and will - * throw an exception. This is to ensure correctness and to fail fast if some ByteBuf implementation is not - * following the expected and supported interface contract. + * {@link ByteBuf#setBytes(int, ByteBuf, int, int)} and {@link ByteBuf#setBytes(int, byte[], int, int)} methods + * and other methods required by getBytes such as {@link ByteBuf#hasArray()}, {@link ByteBuf#hasMemoryAddress()}, + * {@link ByteBuf#nioBufferCount()} and {@link ByteBuf#capacity()}. + * All other methods in the internal ByteBuf implementation are not supported and will throw an exception. + * This is to ensure correctness and to fail fast if some ByteBuf implementation is not following the expected + * and supported interface contract. */ public class ByteBufVisitor { private static final int DEFAULT_VISIT_MAX_DEPTH = 10; @@ -70,6 +70,9 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufV /** * The callback interface for visiting buffers. + * In case of a heap buffer that is backed by an byte[] array, the visitArray method is called. This + * is due to the internal implementation detail of the {@link ByteBuf#getBytes(int, ByteBuf, int, int)} + * method for heap buffers. */ public interface ByteBufVisitorCallback { void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength); From 5cd2dd8b4d4a2bc235d410997e15492b1f328830 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 09:49:24 +0200 Subject: [PATCH 19/39] Refactor to use stateless visitor so that instance can be shared --- .../proto/checksum/DigestManager.java | 48 ++++--- .../bookkeeper/util/ByteBufVisitor.java | 133 +++++++++++------- 2 files changed, 109 insertions(+), 72 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 01d4d0ed11b..1b69b997db1 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -53,6 +53,7 @@ public abstract class DigestManager { final long ledgerId; final boolean useV2Protocol; private final ByteBufAllocator allocator; + private final MutableIntByteBufVisitorCallback byteBufVisitorCallback; abstract int getMacCodeLength(); @@ -62,28 +63,7 @@ public abstract class DigestManager { final int update(int digest, ByteBuf buffer, int offset, int len) { MutableInt digestRef = new MutableInt(digest); - ByteBufVisitor.visitBuffers(buffer, offset, len, - new ByteBufVisitor.ByteBufVisitorCallback() { - @Override - public void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { - if (visitLength > 0) { - // recursively visit the sub buffer and update the digest - int updatedDigest = - internalUpdate(digestRef.intValue(), visitBuffer, visitIndex, visitLength); - digestRef.setValue(updatedDigest); - } - } - - @Override - public void visitArray(byte[] visitArray, int visitIndex, int visitLength) { - if (visitLength > 0) { - // update the digest with the array - int updatedDigest = - internalUpdate(digestRef.intValue(), visitArray, visitIndex, visitLength); - digestRef.setValue(updatedDigest); - } - } - }); + ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); return digestRef.intValue(); } @@ -98,6 +78,7 @@ public DigestManager(long ledgerId, boolean useV2Protocol, ByteBufAllocator allo this.useV2Protocol = useV2Protocol; this.macCodeLength = getMacCodeLength(); this.allocator = allocator; + this.byteBufVisitorCallback = new MutableIntByteBufVisitorCallback(); } public static DigestManager instantiate(long ledgerId, byte[] passwd, DigestType digestType, @@ -371,4 +352,27 @@ public RecoveryData verifyDigestAndReturnLastConfirmed(ByteBuf dataReceived) thr long length = dataReceived.readLong(); return new RecoveryData(lastAddConfirmed, length); } + + private class MutableIntByteBufVisitorCallback implements ByteBufVisitor.ByteBufVisitorCallback { + + @Override + public void visitBuffer(MutableInt digestRef, ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitLength > 0) { + // recursively visit the sub buffer and update the digest + int updatedDigest = + internalUpdate(digestRef.intValue(), visitBuffer, visitIndex, visitLength); + digestRef.setValue(updatedDigest); + } + } + + @Override + public void visitArray(MutableInt digestRef, byte[] visitArray, int visitIndex, int visitLength) { + if (visitLength > 0) { + // update the digest with the array + int updatedDigest = + internalUpdate(digestRef.intValue(), visitArray, visitIndex, visitLength); + digestRef.setValue(updatedDigest); + } + } + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index cdeda13534b..a0c7df0a4e9 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -19,7 +19,6 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; -import io.netty.buffer.Unpooled; import io.netty.util.ByteProcessor; import java.io.IOException; import java.io.InputStream; @@ -63,9 +62,11 @@ public class ByteBufVisitor { * @param offset the offset for the buffer * @param length the length for the buffer * @param callback the callback to call for each visited buffer + * @param context the context to pass to the callback */ - public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback) { - visitBuffers(buffer, offset, length, callback, DEFAULT_VISIT_MAX_DEPTH); + public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, + T context) { + visitBuffers(buffer, offset, length, callback, context, DEFAULT_VISIT_MAX_DEPTH); } /** @@ -74,88 +75,120 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufV * is due to the internal implementation detail of the {@link ByteBuf#getBytes(int, ByteBuf, int, int)} * method for heap buffers. */ - public interface ByteBufVisitorCallback { - void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength); - void visitArray(byte[] visitArray, int visitIndex, int visitLength); + public interface ByteBufVisitorCallback { + void visitBuffer(T context, ByteBuf visitBuffer, int visitIndex, int visitLength); + void visitArray(T context, byte[] visitArray, int visitIndex, int visitLength); } /** - * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback)}. This method allows to specify + * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback, Object)}. This method allows to specify * the maximum depth of recursion for visiting wrapped buffers. */ - public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, - int maxDepth) { - doRecursivelyVisitBuffers(buffer, offset, length, callback, maxDepth, 0); + public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, + T context, int maxDepth) { + InternalContext internalContext = new InternalContext<>(); + internalContext.depth = 0; + internalContext.maxDepth = maxDepth; + internalContext.userContext = context; + internalContext.userCallback = callback; + doRecursivelyVisitBuffers(buffer, offset, length, InternalContextByteBufVisitorCallback.get(), internalContext); } - private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, int length, - ByteBufVisitorCallback callback, int maxDepth, int depth) { + private static class InternalContext { + int depth; + int maxDepth; + ByteBuf parentBuffer; + int parentOffset; + int parentLength; + T userContext; + ByteBufVisitorCallback userCallback; + } + + private static class InternalContextByteBufVisitorCallback + implements ByteBufVisitorCallback> { + static final InternalContextByteBufVisitorCallback INSTANCE = new InternalContextByteBufVisitorCallback<>(); + + static InternalContextByteBufVisitorCallback get() { + @SuppressWarnings("unchecked") + InternalContextByteBufVisitorCallback instance = (InternalContextByteBufVisitorCallback) INSTANCE; + return instance; + } + + @Override + public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, int visitIndex, + int visitLength) { + if (internalContext.depth > 0 && visitBuffer == internalContext.parentBuffer + && visitIndex == internalContext.parentOffset && visitLength == internalContext.parentLength) { + // visit the buffer since it was already passed to visitBuffersImpl and further recursion + // would cause unnecessary recursion up to the max depth of recursion + internalContext.userCallback.visitBuffer(internalContext.userContext, visitBuffer, + visitIndex, visitLength); + } else { + // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively + doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, this, internalContext); + } + } + + @Override + public void visitArray(InternalContext internalContext, byte[] visitArray, int visitIndex, + int visitLength) { + // visit the array + internalContext.userCallback.visitArray(internalContext.userContext, visitArray, visitIndex, + visitLength); + } + } + + private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, int length, + ByteBufVisitorCallback> callback, + InternalContext internalContext) { if (length == 0) { // skip visiting empty buffers return; } // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address // and the max depth has not been reached - if (depth < maxDepth && !buffer.hasMemoryAddress() && !buffer.hasArray()) { - visitBuffersImpl(buffer, offset, length, new ByteBufVisitorCallback() { - @Override - public void visitBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { - if (visitBuffer == buffer && visitIndex == offset && visitLength == length) { - // visit the buffer since it was already passed to visitBuffersImpl and further recursion - // would cause unnecessary recursion up to the max depth of recursion - callback.visitBuffer(visitBuffer, visitIndex, visitLength); - } else { - // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively - doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, callback, maxDepth, depth + 1); - } - } - - @Override - public void visitArray(byte[] visitArray, int visitIndex, int visitLength) { - // visit the array - callback.visitArray(visitArray, visitIndex, visitLength); - } - }); + if (internalContext.depth < internalContext.maxDepth && !buffer.hasMemoryAddress() && !buffer.hasArray()) { + internalContext.parentBuffer = buffer; + internalContext.parentOffset = offset; + internalContext.parentLength = length; + visitBuffersImpl(buffer, offset, length, callback, internalContext); } else { // visit the buffer - callback.visitBuffer(buffer, offset, length); + internalContext.userCallback.visitBuffer(internalContext.userContext, buffer, offset, length); } } // Implementation for visiting a single buffer level using {@link ByteBuf#getBytes(int, ByteBuf, int, int)} - private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, int parentLength, - ByteBufVisitorCallback callback) { + private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, int parentLength, + ByteBufVisitorCallback> callback, + InternalContext internalContext) { // call getBytes to trigger the wrapped buffer visit - parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback), 0, parentLength); + parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback, internalContext), 0, parentLength); } /** * A ByteBuf implementation that can be used as the destination buffer for * a {@link ByteBuf#getBytes(int, ByteBuf)} for visiting the wrapped child buffers. */ - static class GetBytesCallbackByteBuf extends ByteBuf { - private final ByteBufVisitorCallback callback; - private final int capacity; - - GetBytesCallbackByteBuf(ByteBufVisitorCallback callback) { - this(callback, Integer.MAX_VALUE); - } + static class GetBytesCallbackByteBuf extends ByteBuf { + private final ByteBufVisitorCallback> callback; + private final InternalContext internalContext; - GetBytesCallbackByteBuf(ByteBufVisitorCallback callback, int capacity) { + GetBytesCallbackByteBuf(ByteBufVisitorCallback> callback, + InternalContext internalContext) { this.callback = callback; - this.capacity = capacity; + this.internalContext = internalContext; } - @Override public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { - callback.visitBuffer(src, srcIndex, length); + callback.visitBuffer(internalContext, src, srcIndex, length); return this; } @Override public ByteBuf setBytes(int index, byte[] src, int srcIndex, int length) { - callback.visitBuffer(Unpooled.wrappedBuffer(src), srcIndex, length); + internalContext.userCallback.visitArray(internalContext.userContext, src, srcIndex, length); return this; } @@ -180,10 +213,9 @@ public int nioBufferCount() { @Override public int capacity() { // should return sufficient capacity for the total length - return capacity; + return Integer.MAX_VALUE; } - @Override public ByteBuf capacity(int newCapacity) { throw new UnsupportedOperationException(); @@ -1069,4 +1101,5 @@ public boolean equals(Object obj) { return obj == this; } } + } From fc93b5e203b09ad779a51c52c1c15ff60a5c3763 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 2 Feb 2024 13:18:12 +0200 Subject: [PATCH 20/39] Improve test so that a single scenario can be used for debugging --- ...ompositeByteBufUnwrapBugReproduceTest.java | 66 ++++++++++++++----- 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index ba670253af7..f4ef4f18a8c 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -19,7 +19,6 @@ package org.apache.bookkeeper.proto.checksum; import static org.junit.jupiter.api.Assertions.assertArrayEquals; - import com.scurrilous.circe.checksum.IntHash; import com.scurrilous.circe.checksum.Java8IntHash; import com.scurrilous.circe.checksum.Java9IntHash; @@ -33,9 +32,11 @@ import io.netty.util.ReferenceCounted; import java.util.Arrays; import java.util.Collection; +import java.util.List; import org.apache.bookkeeper.proto.BookieProtoEncoding; import org.apache.bookkeeper.proto.BookieProtocol; import org.apache.bookkeeper.util.ByteBufList; +import org.apache.bookkeeper.util.ByteBufVisitor; import org.apache.commons.lang3.RandomUtils; import org.junit.Test; import org.junit.runner.RunWith; @@ -56,22 +57,30 @@ @RunWith(Parameterized.class) public class CompositeByteBufUnwrapBugReproduceTest { final byte[] testPayLoad; - final int bufferPrefixLength; + final int defaultBufferPrefixLength; private final boolean useV2Protocol; + // set to 0 to 3 to run a single scenario for debugging purposes + private static final int RUN_SINGLE_SCENARIO_FOR_DEBUGGING = -1; + @Parameterized.Parameters public static Collection testScenarios() { - return Arrays.asList(new Object[][] { + List scenarios = Arrays.asList(new Object[][] { {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD - 1, true}, {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD - 1, false}, {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, true}, {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, false} }); + if (RUN_SINGLE_SCENARIO_FOR_DEBUGGING >= 0) { + // pick a single scenario for debugging + scenarios = scenarios.subList(RUN_SINGLE_SCENARIO_FOR_DEBUGGING, 1); + } + return scenarios; } public CompositeByteBufUnwrapBugReproduceTest(int payloadSize, boolean useV2Protocol) { this.testPayLoad = createTestPayLoad(payloadSize); - this.bufferPrefixLength = payloadSize / 7; + this.defaultBufferPrefixLength = payloadSize / 7; this.useV2Protocol = useV2Protocol; } @@ -135,31 +144,53 @@ public void shouldCalculateChecksumForCompositeBuffer() { } private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf payload, byte[] referenceOutput) { - assertDigestAndPackageScenario(intHash, payload.retainedDuplicate(), referenceOutput, + assertDigestAndPackageScenario(intHash, payload.retainedDuplicate(), referenceOutput, testPayLoad, "plain payload, no wrapping"); - ByteBuf payload2 = wrapWithPrefixAndCompositeByteBufWithReaderIndexState(payload.retainedDuplicate()); - assertDigestAndPackageScenario(intHash, payload2, referenceOutput, + ByteBuf payload2 = wrapWithPrefixAndCompositeByteBufWithReaderIndexState(payload.retainedDuplicate(), + defaultBufferPrefixLength); + assertDigestAndPackageScenario(intHash, payload2, referenceOutput, testPayLoad, "payload with prefix wrapped in CompositeByteBuf with readerIndex state"); ByteBuf payload3 = wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate( - payload.retainedDuplicate()); - assertDigestAndPackageScenario(intHash, payload3, referenceOutput, + payload.retainedDuplicate(), defaultBufferPrefixLength); + assertDigestAndPackageScenario(intHash, payload3, referenceOutput, testPayLoad, "payload with prefix wrapped in 2 layers of CompositeByteBuf with readerIndex state in the outer " + "composite. In addition, the outer composite is duplicated twice."); - ByteBuf payload4 = wrapInCompositeByteBufAndSlice(payload.retainedDuplicate()); - assertDigestAndPackageScenario(intHash, payload4, referenceOutput, + ByteBuf payload4 = wrapInCompositeByteBufAndSlice(payload.retainedDuplicate(), defaultBufferPrefixLength); + assertDigestAndPackageScenario(intHash, payload4, referenceOutput, testPayLoad, "payload with prefix wrapped in CompositeByteBuf and sliced"); } - private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, + private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, byte[] testPayLoadArray, String scenario) { // this validates that the readable bytes in the payload match the TEST_PAYLOAD content - assertArrayEquals(ByteBufUtil.getBytes(payload.duplicate()), testPayLoad, + assertArrayEquals(testPayLoadArray, ByteBufUtil.getBytes(payload.duplicate()), "input is invalid for scenario '" + scenario + "'"); + + ByteBuf visitedCopy = Unpooled.buffer(payload.readableBytes()); + ByteBufVisitor.visitBuffers(payload, payload.readerIndex(), payload.readableBytes(), new ByteBufVisitor.ByteBufVisitorCallback() { + @Override + public void visitBuffer(Void context, ByteBuf visitBuffer, int visitIndex, int visitLength) { + visitedCopy.writeBytes(visitBuffer, visitIndex, visitLength); + } + + @Override + public void visitArray(Void context, byte[] visitArray, int visitIndex, int visitLength) { + visitedCopy.writeBytes(visitArray, visitIndex, visitLength); + } + }, null); + + assertArrayEquals(ByteBufUtil.getBytes(visitedCopy), testPayLoadArray, + "visited copy is invalid for scenario '" + scenario + "'. Bug in ByteBufVisitor?"); + // compute the digest and package - byte[] output = computeDigestAndPackageForSending(intHash, payload); + byte[] output = computeDigestAndPackageForSending(intHash, payload.duplicate()); + if (referenceOutput == null) { + referenceOutput = + computeDigestAndPackageForSending(new Java8IntHash(), Unpooled.wrappedBuffer(testPayLoadArray)); + } // this validates that the output matches the reference output assertArrayEquals(referenceOutput, output, "output is invalid for scenario '" + scenario + "'"); } @@ -172,7 +203,7 @@ private byte[] computeDigestAndPackageForSending(IntHash intHash, ByteBuf data) return packagedBufferToBytes(packagedBuffer); } - ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload) { + ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload, int bufferPrefixLength) { // create a new buffer with a prefix and the actual payload ByteBuf prefixedPayload = ByteBufAllocator.DEFAULT.buffer(bufferPrefixLength + payload.readableBytes()); prefixedPayload.writeBytes(RandomUtils.nextBytes(bufferPrefixLength)); @@ -188,7 +219,8 @@ ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload) { return outerComposite; } - ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(ByteBuf payload) { + ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(ByteBuf payload, + int bufferPrefixLength) { // create a new buffer with a prefix and the actual payload ByteBuf prefixedPayload = ByteBufAllocator.DEFAULT.buffer(bufferPrefixLength + payload.readableBytes()); prefixedPayload.writeBytes(RandomUtils.nextBytes(bufferPrefixLength)); @@ -209,7 +241,7 @@ ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultiple return outerComposite.duplicate().duplicate(); } - ByteBuf wrapInCompositeByteBufAndSlice(ByteBuf payload) { + ByteBuf wrapInCompositeByteBufAndSlice(ByteBuf payload, int bufferPrefixLength) { // create a composite buffer CompositeByteBuf compositeWithPrefix = ByteBufAllocator.DEFAULT.compositeBuffer(); compositeWithPrefix.addComponent(true, Unpooled.wrappedBuffer(RandomUtils.nextBytes(bufferPrefixLength))); From 272e962930a31cbc237c5e7c0bd0c93213520ba4 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 2 Feb 2024 13:00:24 +0200 Subject: [PATCH 21/39] Fix bug in Java9IntHash calculation that assumed crc32c_update(x) == ~crc32c_update(~x) - Java9IntHash uses private methods from java.util.zip.CRC32C class, updateBytes and updateDirectByteBuffer. When inspecting the use and interface contract, it doesn't match how it is used in Java9IntHash. This PR addresses that by introducing a separate initial value for initializing the accumulated value so that the initial value could match the logic in java.util.zip.CRC32C.reset method. There's also a separate method for finalizing the accumulated value into a final checksum value. This is to match the java.util.zip.CRC32C.getValue method's logic (uses bitwise complement operator ~). - With a quick glance, it might appear that the previous logic is similar. However it isn't since I have a failing test which gets fixed with this change. I haven't yet added the Java9IntHash level unit test case to prove how it differs. It must be related to integer value overflow. For the CRC32C function, I believe it means that it cannot be assumed in all cases that func(x) == ~func(~x). That's the assumption that the previous code was making. It probably applies for many inputs, but not all. It would break in overflow cases. --- .../proto/checksum/CRC32CDigestManager.java | 12 ++++++- .../proto/checksum/DigestManager.java | 35 ++++++++++++++----- ...ompositeByteBufUnwrapBugReproduceTest.java | 10 ++++++ .../circe/checksum/Crc32cIntChecksum.java | 7 ++++ .../scurrilous/circe/checksum/IntHash.java | 8 +++++ .../circe/checksum/Java9IntHash.java | 25 ++++++++----- .../circe/checksum/ChecksumTest.java | 24 +++++++++---- .../circe/checksum/Java9IntHashTest.java | 8 ++--- 8 files changed, 101 insertions(+), 28 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java index 0b9c469beb1..9d07f0feec0 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java @@ -54,4 +54,14 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return Crc32cIntChecksum.resumeChecksum(digest, buffer, offset, len); } -} + + @Override + protected int initialDigest() { + return Crc32cIntChecksum.initialValue(); + } + + @Override + protected int finalizeDigest(int digest) { + return Crc32cIntChecksum.finalizeValue(digest); + } +} \ No newline at end of file diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 1b69b997db1..f1e8e6ecf85 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -61,12 +61,31 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, byte[] buffer, int offset, int len); + protected int initialDigest() { + return 0; + }; + + protected int finalizeDigest(int digest) { + return digest; + } + final int update(int digest, ByteBuf buffer, int offset, int len) { + if (len == 0) { + return digest; + } MutableInt digestRef = new MutableInt(digest); ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); return digestRef.intValue(); } + protected final int initializeAndUpdate(ByteBuf buf, int offset, int len) { + return update(initialDigest(), buf, offset, len); + } + + private int updateAndFinalize(ByteBuf data, int digest, int offset, int len) { + return finalizeDigest(update(digest, data, offset, len)); + } + abstract void populateValueAndReset(int digest, ByteBuf buffer); abstract boolean isInt32Digest(); @@ -145,8 +164,8 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long buf.writeLong(length); // Compute checksum over the headers - int digest = update(0, buf, buf.readerIndex(), buf.readableBytes()); - digest = update(digest, data, data.readerIndex(), data.readableBytes()); + int digest = initializeAndUpdate(buf, buf.readerIndex(), buf.readableBytes()); + digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, buf); @@ -170,8 +189,8 @@ private ByteBufList computeDigestAndPackageForSendingV3(long entryId, long lastA headersBuffer.writeLong(lastAddConfirmed); headersBuffer.writeLong(length); - int digest = update(0, headersBuffer, 0, METADATA_LENGTH); - digest = update(digest, data, data.readerIndex(), data.readableBytes()); + int digest = initializeAndUpdate(headersBuffer, 0, METADATA_LENGTH); + digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, headersBuffer); return ByteBufList.get(headersBuffer, data); } @@ -193,7 +212,7 @@ public ByteBufList computeDigestAndPackageForSendingLac(long lac) { headersBuffer.writeLong(ledgerId); headersBuffer.writeLong(lac); - int digest = update(0, headersBuffer, 0, LAC_METADATA_LENGTH); + int digest = finalizeDigest(initializeAndUpdate(headersBuffer, 0, LAC_METADATA_LENGTH)); populateValueAndReset(digest, headersBuffer); return ByteBufList.get(headersBuffer); @@ -229,10 +248,10 @@ private void verifyDigest(long entryId, ByteBuf dataReceived, boolean skipEntryI this.getClass().getName(), dataReceived.readableBytes()); throw new BKDigestMatchException(); } - int digest = update(0, dataReceived, 0, METADATA_LENGTH); + int digest = initializeAndUpdate(dataReceived, 0, METADATA_LENGTH); int offset = METADATA_LENGTH + macCodeLength; - digest = update(digest, dataReceived, offset, dataReceived.readableBytes() - offset); + digest = updateAndFinalize(dataReceived, digest, offset, dataReceived.readableBytes() - offset); if (isInt32Digest()) { int receivedDigest = dataReceived.getInt(METADATA_LENGTH); @@ -277,7 +296,7 @@ public long verifyDigestAndReturnLac(ByteBuf dataReceived) throws BKDigestMatchE throw new BKDigestMatchException(); } - int digest = update(0, dataReceived, 0, LAC_METADATA_LENGTH); + int digest = finalizeDigest(initializeAndUpdate(dataReceived, 0, LAC_METADATA_LENGTH)); if (isInt32Digest()) { int receivedDigest = dataReceived.getInt(LAC_METADATA_LENGTH); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index f4ef4f18a8c..4ce2910bf2c 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -129,6 +129,16 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return intHash.resume(digest, buffer, offset, len); } + + @Override + protected int initialDigest() { + return intHash.initialValue(); + } + + @Override + protected int finalizeDigest(int digest) { + return intHash.finalizeValue(digest); + } } @Test diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java index 0f0bcbe4528..27dab5a9afb 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java @@ -94,4 +94,11 @@ public static int resumeChecksum(int previousChecksum, byte[] payload, int offse return CRC32C_HASH.resume(previousChecksum, payload, offset, len); } + public static int initialValue() { + return CRC32C_HASH.initialValue(); + } + + public static int finalizeValue(int current) { + return CRC32C_HASH.finalizeValue(current); + } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java index 84fb1c73364..92ab93f7862 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java @@ -30,4 +30,12 @@ public interface IntHash { int resume(int current, ByteBuf buffer, int offset, int len); int resume(int current, byte[] buffer, int offset, int len); + + default int initialValue() { + return 0; + } + + default int finalizeValue(int current) { + return current; + } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index dc2ec58ef79..28dbfaedc21 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -68,12 +68,12 @@ protected byte[] initialValue() { @Override public int calculate(ByteBuf buffer) { - return resume(0, buffer); + return finalizeValue(resume(initialValue(), buffer)); } @Override public int calculate(ByteBuf buffer, int offset, int len) { - return resume(0, buffer, offset, len); + return finalizeValue(resume(initialValue(), buffer, offset, len)); } private int resume(int current, long address, int offset, int length) { @@ -100,13 +100,11 @@ public int resume(int current, ByteBuf buffer) { @Override public int resume(int current, ByteBuf buffer, int offset, int len) { - int negCrc = ~current; - if (buffer.hasMemoryAddress()) { - negCrc = resume(negCrc, buffer.memoryAddress(), offset, len); + current = resume(current, buffer.memoryAddress(), offset, len); } else if (buffer.hasArray()) { int arrayOffset = buffer.arrayOffset() + offset; - negCrc = resume(negCrc, buffer.array(), arrayOffset, len); + current = resume(current, buffer.array(), arrayOffset, len); } else { byte[] b = TL_BUFFER.get(); int toRead = len; @@ -114,12 +112,23 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { while (toRead > 0) { int length = Math.min(toRead, b.length); buffer.slice(loopOffset, length).readBytes(b, 0, length); - negCrc = resume(negCrc, b, 0, length); + current = resume(current, b, 0, length); toRead -= length; loopOffset += length; } } + return current; + } + + @Override + public int initialValue() { + // match java.util.zip.CRC32C.reset logic + return 0xFFFFFFFF; + } - return ~negCrc; + @Override + public int finalizeValue(int current) { + // match java.util.zip.CRC32C.getValue logic + return ~current; } } diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java index f68b362a1af..b8283fd633b 100644 --- a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java +++ b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ + package com.scurrilous.circe.checksum; import static com.scurrilous.circe.params.CrcParameters.CRC32C; @@ -44,7 +45,9 @@ public void testCrc32cValue() { @Test public void testCrc32cValueResume() { final byte[] bytes = "Some String".getBytes(); - int checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length); + int checksum = Crc32cIntChecksum.finalizeValue( + Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0, + bytes.length)); assertEquals(608512271, checksum); } @@ -56,22 +59,26 @@ public void testCrc32cValueIncremental() { int checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes)); assertEquals(608512271, checksum); - checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 1)); + checksum = + Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 1)); for (int i = 1; i < bytes.length; i++) { checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), i, 1); } + checksum = Crc32cIntChecksum.finalizeValue(checksum); assertEquals(608512271, checksum); - checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 4)); + checksum = + Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 4)); checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), 4, 7); + checksum = Crc32cIntChecksum.finalizeValue(checksum); assertEquals(608512271, checksum); ByteBuf buffer = Unpooled.wrappedBuffer(bytes, 0, 4); - checksum = Crc32cIntChecksum.computeChecksum(buffer); + checksum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), buffer); checksum = Crc32cIntChecksum.resumeChecksum( - checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4); - + checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4); + checksum = Crc32cIntChecksum.finalizeValue(checksum); assertEquals(608512271, checksum); } @@ -86,7 +93,10 @@ public void testCrc32cLongValue() { @Test public void testCrc32cLongValueResume() { final byte[] bytes = "Some String".getBytes(); - long checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length); + long checksum = + Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0, + bytes.length); + checksum = Crc32cIntChecksum.finalizeValue((int) checksum); assertEquals(608512271L, checksum); } diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java index 3fb57b4a1aa..69c54af8e60 100644 --- a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java +++ b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java @@ -65,8 +65,8 @@ public void calculateCheckSumUsingCompositeByteBuf() { int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal); // Calculate: case-2. - int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1); - int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2); + int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1); + int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2)); // Verify: the results of both ways to calculate the checksum are same. Assert.assertEquals(checksumRes1, checksumRes2); @@ -90,8 +90,8 @@ public void calculateCheckSumUsingNoArrayNoMemoryAddrByteBuf() { int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal); // Calculate: case-2. - int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1); - int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2); + int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1); + int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2)); // Verify: the results of both ways to calculate the checksum are same. Assert.assertEquals(checksumRes1, checksumRes2); From b4d0b1859ed680bfae318af163b28850a8773a36 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 2 Feb 2024 17:11:01 +0200 Subject: [PATCH 22/39] Fix checkstyle --- ...ompositeByteBufUnwrapBugReproduceTest.java | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 4ce2910bf2c..206104031f2 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ + package org.apache.bookkeeper.proto.checksum; import static org.junit.jupiter.api.Assertions.assertArrayEquals; @@ -71,7 +72,7 @@ public static Collection testScenarios() { {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, true}, {BookieProtoEncoding.SMALL_ENTRY_SIZE_THRESHOLD, false} }); - if (RUN_SINGLE_SCENARIO_FOR_DEBUGGING >= 0) { + if (RUN_SINGLE_SCENARIO_FOR_DEBUGGING >= 0) { // pick a single scenario for debugging scenarios = scenarios.subList(RUN_SINGLE_SCENARIO_FOR_DEBUGGING, 1); } @@ -173,24 +174,26 @@ private void assertDigestAndPackageMatchesReference(IntHash intHash, ByteBuf pay "payload with prefix wrapped in CompositeByteBuf and sliced"); } - private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, byte[] testPayLoadArray, + private void assertDigestAndPackageScenario(IntHash intHash, ByteBuf payload, byte[] referenceOutput, + byte[] testPayLoadArray, String scenario) { // this validates that the readable bytes in the payload match the TEST_PAYLOAD content assertArrayEquals(testPayLoadArray, ByteBufUtil.getBytes(payload.duplicate()), "input is invalid for scenario '" + scenario + "'"); ByteBuf visitedCopy = Unpooled.buffer(payload.readableBytes()); - ByteBufVisitor.visitBuffers(payload, payload.readerIndex(), payload.readableBytes(), new ByteBufVisitor.ByteBufVisitorCallback() { - @Override - public void visitBuffer(Void context, ByteBuf visitBuffer, int visitIndex, int visitLength) { - visitedCopy.writeBytes(visitBuffer, visitIndex, visitLength); - } - - @Override - public void visitArray(Void context, byte[] visitArray, int visitIndex, int visitLength) { - visitedCopy.writeBytes(visitArray, visitIndex, visitLength); - } - }, null); + ByteBufVisitor.visitBuffers(payload, payload.readerIndex(), payload.readableBytes(), + new ByteBufVisitor.ByteBufVisitorCallback() { + @Override + public void visitBuffer(Void context, ByteBuf visitBuffer, int visitIndex, int visitLength) { + visitedCopy.writeBytes(visitBuffer, visitIndex, visitLength); + } + + @Override + public void visitArray(Void context, byte[] visitArray, int visitIndex, int visitLength) { + visitedCopy.writeBytes(visitArray, visitIndex, visitLength); + } + }, null); assertArrayEquals(ByteBufUtil.getBytes(visitedCopy), testPayLoadArray, "visited copy is invalid for scenario '" + scenario + "'. Bug in ByteBufVisitor?"); @@ -230,7 +233,7 @@ ByteBuf wrapWithPrefixAndCompositeByteBufWithReaderIndexState(ByteBuf payload, i } ByteBuf wrapWithPrefixAndMultipleCompositeByteBufWithReaderIndexStateAndMultipleLayersOfDuplicate(ByteBuf payload, - int bufferPrefixLength) { + int bufferPrefixLength) { // create a new buffer with a prefix and the actual payload ByteBuf prefixedPayload = ByteBufAllocator.DEFAULT.buffer(bufferPrefixLength + payload.readableBytes()); prefixedPayload.writeBytes(RandomUtils.nextBytes(bufferPrefixLength)); From 9c1b68f949ffa47aa483845fd04a7d318d0838ac Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 2 Feb 2024 17:26:23 +0200 Subject: [PATCH 23/39] Fix checkstyle --- .../proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 206104031f2..cc0ad7ac3ba 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -20,6 +20,7 @@ package org.apache.bookkeeper.proto.checksum; import static org.junit.jupiter.api.Assertions.assertArrayEquals; + import com.scurrilous.circe.checksum.IntHash; import com.scurrilous.circe.checksum.Java8IntHash; import com.scurrilous.circe.checksum.Java9IntHash; From a751294ec740e0697c6501ffe2104133cd6d8098 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 11:35:55 +0200 Subject: [PATCH 24/39] Fix missing depth increment that prevents StackOverflowException --- .../src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java | 1 + 1 file changed, 1 insertion(+) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index a0c7df0a4e9..7fff2af79fb 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -124,6 +124,7 @@ public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, internalContext.userCallback.visitBuffer(internalContext.userContext, visitBuffer, visitIndex, visitLength); } else { + internalContext.depth++; // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, this, internalContext); } From 5f8fef52a2a66e2099cf63d5244c202c6a2dd95e Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 11:43:28 +0200 Subject: [PATCH 25/39] Properly handle the depth increase and decrease --- .../main/java/org/apache/bookkeeper/util/ByteBufVisitor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 7fff2af79fb..60ab7a24282 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -124,7 +124,6 @@ public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, internalContext.userCallback.visitBuffer(internalContext.userContext, visitBuffer, visitIndex, visitLength); } else { - internalContext.depth++; // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, this, internalContext); } @@ -152,7 +151,9 @@ private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, in internalContext.parentBuffer = buffer; internalContext.parentOffset = offset; internalContext.parentLength = length; + internalContext.depth++; visitBuffersImpl(buffer, offset, length, callback, internalContext); + internalContext.depth--; } else { // visit the buffer internalContext.userCallback.visitBuffer(internalContext.userContext, buffer, offset, length); From bb703ab6624d9decc9707200a18a5ffcc9be76fa Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 12:01:26 +0200 Subject: [PATCH 26/39] Remove unnecessary condition --- .../main/java/org/apache/bookkeeper/util/ByteBufVisitor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 60ab7a24282..8a9ba67d2ec 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -117,8 +117,8 @@ static InternalContextByteBufVisitorCallback get() { @Override public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, int visitIndex, int visitLength) { - if (internalContext.depth > 0 && visitBuffer == internalContext.parentBuffer - && visitIndex == internalContext.parentOffset && visitLength == internalContext.parentLength) { + if (visitBuffer == internalContext.parentBuffer && visitIndex == internalContext.parentOffset + && visitLength == internalContext.parentLength) { // visit the buffer since it was already passed to visitBuffersImpl and further recursion // would cause unnecessary recursion up to the max depth of recursion internalContext.userCallback.visitBuffer(internalContext.userContext, visitBuffer, From 2b79716572a54a8bf8361860b9cfbef576d975d9 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 15:12:21 +0200 Subject: [PATCH 27/39] Use more efficient way to read bytes to the target array --- .../main/java/com/scurrilous/circe/checksum/Java9IntHash.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index 28dbfaedc21..1ccfc353957 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -111,7 +111,7 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { int loopOffset = offset; while (toRead > 0) { int length = Math.min(toRead, b.length); - buffer.slice(loopOffset, length).readBytes(b, 0, length); + buffer.getBytes(loopOffset, b, 0, length); current = resume(current, b, 0, length); toRead -= length; loopOffset += length; From f650b9b17a954ba915cf0ac4cd787f861c863b5d Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 15:52:01 +0200 Subject: [PATCH 28/39] Don't use ByteBufVisitor if it's not necessary --- .../bookkeeper/proto/checksum/DigestManager.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index f1e8e6ecf85..8c2988f6f7e 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -73,9 +73,15 @@ final int update(int digest, ByteBuf buffer, int offset, int len) { if (len == 0) { return digest; } - MutableInt digestRef = new MutableInt(digest); - ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); - return digestRef.intValue(); + if (buffer.hasMemoryAddress()) { + return internalUpdate(digest, buffer, offset, len); + } else if (buffer.hasArray()) { + return internalUpdate(digest, buffer.array(), offset, len); + } else { + MutableInt digestRef = new MutableInt(digest); + ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); + return digestRef.intValue(); + } } protected final int initializeAndUpdate(ByteBuf buf, int offset, int len) { From d21a5cddd2e7d83ca6f99a40c99e7c3200c1f12f Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 21:40:38 +0200 Subject: [PATCH 29/39] Revert "Fix bug in Java9IntHash calculation that assumed crc32c_update(x) == ~crc32c_update(~x)" This reverts commit 272e962930a31cbc237c5e7c0bd0c93213520ba4. --- .../proto/checksum/CRC32CDigestManager.java | 12 +------ .../proto/checksum/DigestManager.java | 35 +++++-------------- ...ompositeByteBufUnwrapBugReproduceTest.java | 10 ------ .../circe/checksum/Crc32cIntChecksum.java | 7 ---- .../scurrilous/circe/checksum/IntHash.java | 8 ----- .../circe/checksum/Java9IntHash.java | 25 +++++-------- .../circe/checksum/ChecksumTest.java | 24 ++++--------- .../circe/checksum/Java9IntHashTest.java | 8 ++--- 8 files changed, 28 insertions(+), 101 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java index 9d07f0feec0..0b9c469beb1 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java @@ -54,14 +54,4 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return Crc32cIntChecksum.resumeChecksum(digest, buffer, offset, len); } - - @Override - protected int initialDigest() { - return Crc32cIntChecksum.initialValue(); - } - - @Override - protected int finalizeDigest(int digest) { - return Crc32cIntChecksum.finalizeValue(digest); - } -} \ No newline at end of file +} diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 8c2988f6f7e..cf620db5445 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -61,18 +61,7 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, byte[] buffer, int offset, int len); - protected int initialDigest() { - return 0; - }; - - protected int finalizeDigest(int digest) { - return digest; - } - final int update(int digest, ByteBuf buffer, int offset, int len) { - if (len == 0) { - return digest; - } if (buffer.hasMemoryAddress()) { return internalUpdate(digest, buffer, offset, len); } else if (buffer.hasArray()) { @@ -84,14 +73,6 @@ final int update(int digest, ByteBuf buffer, int offset, int len) { } } - protected final int initializeAndUpdate(ByteBuf buf, int offset, int len) { - return update(initialDigest(), buf, offset, len); - } - - private int updateAndFinalize(ByteBuf data, int digest, int offset, int len) { - return finalizeDigest(update(digest, data, offset, len)); - } - abstract void populateValueAndReset(int digest, ByteBuf buffer); abstract boolean isInt32Digest(); @@ -170,8 +151,8 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long buf.writeLong(length); // Compute checksum over the headers - int digest = initializeAndUpdate(buf, buf.readerIndex(), buf.readableBytes()); - digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes()); + int digest = update(0, buf, buf.readerIndex(), buf.readableBytes()); + digest = update(digest, data, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, buf); @@ -195,8 +176,8 @@ private ByteBufList computeDigestAndPackageForSendingV3(long entryId, long lastA headersBuffer.writeLong(lastAddConfirmed); headersBuffer.writeLong(length); - int digest = initializeAndUpdate(headersBuffer, 0, METADATA_LENGTH); - digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes()); + int digest = update(0, headersBuffer, 0, METADATA_LENGTH); + digest = update(digest, data, data.readerIndex(), data.readableBytes()); populateValueAndReset(digest, headersBuffer); return ByteBufList.get(headersBuffer, data); } @@ -218,7 +199,7 @@ public ByteBufList computeDigestAndPackageForSendingLac(long lac) { headersBuffer.writeLong(ledgerId); headersBuffer.writeLong(lac); - int digest = finalizeDigest(initializeAndUpdate(headersBuffer, 0, LAC_METADATA_LENGTH)); + int digest = update(0, headersBuffer, 0, LAC_METADATA_LENGTH); populateValueAndReset(digest, headersBuffer); return ByteBufList.get(headersBuffer); @@ -254,10 +235,10 @@ private void verifyDigest(long entryId, ByteBuf dataReceived, boolean skipEntryI this.getClass().getName(), dataReceived.readableBytes()); throw new BKDigestMatchException(); } - int digest = initializeAndUpdate(dataReceived, 0, METADATA_LENGTH); + int digest = update(0, dataReceived, 0, METADATA_LENGTH); int offset = METADATA_LENGTH + macCodeLength; - digest = updateAndFinalize(dataReceived, digest, offset, dataReceived.readableBytes() - offset); + digest = update(digest, dataReceived, offset, dataReceived.readableBytes() - offset); if (isInt32Digest()) { int receivedDigest = dataReceived.getInt(METADATA_LENGTH); @@ -302,7 +283,7 @@ public long verifyDigestAndReturnLac(ByteBuf dataReceived) throws BKDigestMatchE throw new BKDigestMatchException(); } - int digest = finalizeDigest(initializeAndUpdate(dataReceived, 0, LAC_METADATA_LENGTH)); + int digest = update(0, dataReceived, 0, LAC_METADATA_LENGTH); if (isInt32Digest()) { int receivedDigest = dataReceived.getInt(LAC_METADATA_LENGTH); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index cc0ad7ac3ba..7cd58290bf7 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -131,16 +131,6 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return intHash.resume(digest, buffer, offset, len); } - - @Override - protected int initialDigest() { - return intHash.initialValue(); - } - - @Override - protected int finalizeDigest(int digest) { - return intHash.finalizeValue(digest); - } } @Test diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java index 27dab5a9afb..0f0bcbe4528 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java @@ -94,11 +94,4 @@ public static int resumeChecksum(int previousChecksum, byte[] payload, int offse return CRC32C_HASH.resume(previousChecksum, payload, offset, len); } - public static int initialValue() { - return CRC32C_HASH.initialValue(); - } - - public static int finalizeValue(int current) { - return CRC32C_HASH.finalizeValue(current); - } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java index 92ab93f7862..84fb1c73364 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java @@ -30,12 +30,4 @@ public interface IntHash { int resume(int current, ByteBuf buffer, int offset, int len); int resume(int current, byte[] buffer, int offset, int len); - - default int initialValue() { - return 0; - } - - default int finalizeValue(int current) { - return current; - } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index 1ccfc353957..87e7f6f1672 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -68,12 +68,12 @@ protected byte[] initialValue() { @Override public int calculate(ByteBuf buffer) { - return finalizeValue(resume(initialValue(), buffer)); + return resume(0, buffer); } @Override public int calculate(ByteBuf buffer, int offset, int len) { - return finalizeValue(resume(initialValue(), buffer, offset, len)); + return resume(0, buffer, offset, len); } private int resume(int current, long address, int offset, int length) { @@ -100,11 +100,13 @@ public int resume(int current, ByteBuf buffer) { @Override public int resume(int current, ByteBuf buffer, int offset, int len) { + int negCrc = ~current; + if (buffer.hasMemoryAddress()) { - current = resume(current, buffer.memoryAddress(), offset, len); + negCrc = resume(negCrc, buffer.memoryAddress(), offset, len); } else if (buffer.hasArray()) { int arrayOffset = buffer.arrayOffset() + offset; - current = resume(current, buffer.array(), arrayOffset, len); + negCrc = resume(negCrc, buffer.array(), arrayOffset, len); } else { byte[] b = TL_BUFFER.get(); int toRead = len; @@ -112,23 +114,12 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { while (toRead > 0) { int length = Math.min(toRead, b.length); buffer.getBytes(loopOffset, b, 0, length); - current = resume(current, b, 0, length); + negCrc = resume(negCrc, b, 0, length); toRead -= length; loopOffset += length; } } - return current; - } - - @Override - public int initialValue() { - // match java.util.zip.CRC32C.reset logic - return 0xFFFFFFFF; - } - @Override - public int finalizeValue(int current) { - // match java.util.zip.CRC32C.getValue logic - return ~current; + return ~negCrc; } } diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java index b8283fd633b..f68b362a1af 100644 --- a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java +++ b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ - package com.scurrilous.circe.checksum; import static com.scurrilous.circe.params.CrcParameters.CRC32C; @@ -45,9 +44,7 @@ public void testCrc32cValue() { @Test public void testCrc32cValueResume() { final byte[] bytes = "Some String".getBytes(); - int checksum = Crc32cIntChecksum.finalizeValue( - Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0, - bytes.length)); + int checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length); assertEquals(608512271, checksum); } @@ -59,26 +56,22 @@ public void testCrc32cValueIncremental() { int checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes)); assertEquals(608512271, checksum); - checksum = - Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 1)); + checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 1)); for (int i = 1; i < bytes.length; i++) { checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), i, 1); } - checksum = Crc32cIntChecksum.finalizeValue(checksum); assertEquals(608512271, checksum); - checksum = - Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 4)); + checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 4)); checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), 4, 7); - checksum = Crc32cIntChecksum.finalizeValue(checksum); assertEquals(608512271, checksum); ByteBuf buffer = Unpooled.wrappedBuffer(bytes, 0, 4); - checksum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), buffer); + checksum = Crc32cIntChecksum.computeChecksum(buffer); checksum = Crc32cIntChecksum.resumeChecksum( - checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4); - checksum = Crc32cIntChecksum.finalizeValue(checksum); + checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4); + assertEquals(608512271, checksum); } @@ -93,10 +86,7 @@ public void testCrc32cLongValue() { @Test public void testCrc32cLongValueResume() { final byte[] bytes = "Some String".getBytes(); - long checksum = - Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0, - bytes.length); - checksum = Crc32cIntChecksum.finalizeValue((int) checksum); + long checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length); assertEquals(608512271L, checksum); } diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java index 69c54af8e60..3fb57b4a1aa 100644 --- a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java +++ b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java @@ -65,8 +65,8 @@ public void calculateCheckSumUsingCompositeByteBuf() { int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal); // Calculate: case-2. - int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1); - int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2)); + int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1); + int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2); // Verify: the results of both ways to calculate the checksum are same. Assert.assertEquals(checksumRes1, checksumRes2); @@ -90,8 +90,8 @@ public void calculateCheckSumUsingNoArrayNoMemoryAddrByteBuf() { int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal); // Calculate: case-2. - int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1); - int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2)); + int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1); + int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2); // Verify: the results of both ways to calculate the checksum are same. Assert.assertEquals(checksumRes1, checksumRes2); From a9ed5e47a59b4d7a6d3db892fe137712f8eb61ac Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 3 Feb 2024 21:59:01 +0200 Subject: [PATCH 30/39] Fix issue in resume byte[] version that was added - input and output should be complemented. explanation has been added to the resume ByteBuf method --- .../circe/checksum/Java9IntHash.java | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index 87e7f6f1672..f615df7e069 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -76,7 +76,7 @@ public int calculate(ByteBuf buffer, int offset, int len) { return resume(0, buffer, offset, len); } - private int resume(int current, long address, int offset, int length) { + private int updateDirectByteBuffer(int current, long address, int offset, int length) { try { return (int) UPDATE_DIRECT_BYTEBUFFER.invoke(null, current, address, offset, offset + length); } catch (IllegalAccessException | InvocationTargetException e) { @@ -86,6 +86,13 @@ private int resume(int current, long address, int offset, int length) { @Override public int resume(int current, byte[] array, int offset, int length) { + // the bit-wise complementing of the input and output is explained in the resume method below + current = ~current; + current = updateBytes(current, array, offset, length); + return ~current; + } + + private static int updateBytes(int current, byte[] array, int offset, int length) { try { return (int) UPDATE_BYTES.invoke(null, current, array, offset, offset + length); } catch (IllegalAccessException | InvocationTargetException e) { @@ -100,13 +107,22 @@ public int resume(int current, ByteBuf buffer) { @Override public int resume(int current, ByteBuf buffer, int offset, int len) { - int negCrc = ~current; + // flip the bits (bit-wise complements) for the input value. + // this serves two purposes: + // - the CRC32C algorithm is designed to start with a seed value of all bits set to 1 (0xffffffff) + // - when 0 is passed in initially, ~0 will result in the correct initial value (0xfffffff). + // - the CRC32C algorithm is designed in a way that the final value is complemented as the last step. + // - this method will always complement the return value + // - for iterative use, the input value should be complemented to continue calculations. + // This way the algorithm can be used incrementally without a separate initialization step + // and finalization step. + current = ~current; if (buffer.hasMemoryAddress()) { - negCrc = resume(negCrc, buffer.memoryAddress(), offset, len); + current = updateDirectByteBuffer(current, buffer.memoryAddress(), offset, len); } else if (buffer.hasArray()) { int arrayOffset = buffer.arrayOffset() + offset; - negCrc = resume(negCrc, buffer.array(), arrayOffset, len); + current = updateBytes(current, buffer.array(), arrayOffset, len); } else { byte[] b = TL_BUFFER.get(); int toRead = len; @@ -114,12 +130,14 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { while (toRead > 0) { int length = Math.min(toRead, b.length); buffer.getBytes(loopOffset, b, 0, length); - negCrc = resume(negCrc, b, 0, length); + current = updateBytes(current, b, 0, length); toRead -= length; loopOffset += length; } } - return ~negCrc; + // return a complement of the current value to match the CRC32C algorithm's finalization step. + // if there's another resume step, the value will be complemented to start the next step. + return ~current; } } From b7d5795ea4a64190edb48ec0a52dca9c7285f008 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 11:01:46 +0200 Subject: [PATCH 31/39] Polish ByteBufVisitor - reuse GetBytesCallbackByteBuf instance for handling the root ByteBuf instance --- .../bookkeeper/util/ByteBufVisitor.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 8a9ba67d2ec..d76905285bc 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -91,7 +91,7 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, Byte internalContext.maxDepth = maxDepth; internalContext.userContext = context; internalContext.userCallback = callback; - doRecursivelyVisitBuffers(buffer, offset, length, InternalContextByteBufVisitorCallback.get(), internalContext); + recursivelyVisitBuffers(buffer, offset, length, internalContext); } private static class InternalContext { @@ -102,6 +102,8 @@ private static class InternalContext { int parentLength; T userContext; ByteBufVisitorCallback userCallback; + GetBytesCallbackByteBuf callbackByteBuf = new GetBytesCallbackByteBuf(this); + InternalContextByteBufVisitorCallback callback = InternalContextByteBufVisitorCallback.get(); } private static class InternalContextByteBufVisitorCallback @@ -125,7 +127,7 @@ public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, visitIndex, visitLength); } else { // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively - doRecursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, this, internalContext); + recursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, internalContext); } } @@ -138,9 +140,8 @@ public void visitArray(InternalContext internalContext, byte[] visitArray, in } } - private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, int length, - ByteBufVisitorCallback> callback, - InternalContext internalContext) { + private static void recursivelyVisitBuffers(ByteBuf buffer, int offset, int length, + InternalContext internalContext) { if (length == 0) { // skip visiting empty buffers return; @@ -152,7 +153,7 @@ private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, in internalContext.parentOffset = offset; internalContext.parentLength = length; internalContext.depth++; - visitBuffersImpl(buffer, offset, length, callback, internalContext); + visitBuffersImpl(buffer, offset, length, internalContext); internalContext.depth--; } else { // visit the buffer @@ -162,10 +163,9 @@ private static void doRecursivelyVisitBuffers(ByteBuf buffer, int offset, in // Implementation for visiting a single buffer level using {@link ByteBuf#getBytes(int, ByteBuf, int, int)} private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, int parentLength, - ByteBufVisitorCallback> callback, - InternalContext internalContext) { + InternalContext internalContext) { // call getBytes to trigger the wrapped buffer visit - parentBuffer.getBytes(parentOffset, new GetBytesCallbackByteBuf(callback, internalContext), 0, parentLength); + parentBuffer.getBytes(parentOffset, internalContext.callbackByteBuf, 0, parentLength); } /** @@ -173,18 +173,15 @@ private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, * a {@link ByteBuf#getBytes(int, ByteBuf)} for visiting the wrapped child buffers. */ static class GetBytesCallbackByteBuf extends ByteBuf { - private final ByteBufVisitorCallback> callback; private final InternalContext internalContext; - GetBytesCallbackByteBuf(ByteBufVisitorCallback> callback, - InternalContext internalContext) { - this.callback = callback; + GetBytesCallbackByteBuf(InternalContext internalContext) { this.internalContext = internalContext; } @Override public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { - callback.visitBuffer(internalContext, src, srcIndex, length); + internalContext.callback.visitBuffer(internalContext, src, srcIndex, length); return this; } From 34c1c8a4d59a3840504775775354566e1a1bdf1e Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 12:31:17 +0200 Subject: [PATCH 32/39] Use extracted method --- .../bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java index f0814a72130..07a2bdf464f 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java @@ -70,7 +70,7 @@ private static int updateArray(int crcValue, byte[] buf, int offset, int length) @Override public void update(byte[] buffer, int offset, int len) { try { - crcValue = (int) updateBytes.invoke(null, crcValue, buffer, offset, len); + crcValue = updateArray(crcValue, buffer, offset, len); } catch (IllegalAccessException | InvocationTargetException e) { throw new RuntimeException(e); } From be2094849ca7dc5774e2a7f0ee68ca7cbb92cec8 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 12:31:32 +0200 Subject: [PATCH 33/39] Fix bug with array handling --- .../org/apache/bookkeeper/proto/checksum/DigestManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index cf620db5445..c6d9f1abe76 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -65,7 +65,7 @@ final int update(int digest, ByteBuf buffer, int offset, int len) { if (buffer.hasMemoryAddress()) { return internalUpdate(digest, buffer, offset, len); } else if (buffer.hasArray()) { - return internalUpdate(digest, buffer.array(), offset, len); + return internalUpdate(digest, buffer.array(), buffer.arrayOffset() + offset, len); } else { MutableInt digestRef = new MutableInt(digest); ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); From 799865987eb0ae865a1886d113ba13ae3d78f6b3 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 17:21:22 +0200 Subject: [PATCH 34/39] Polish ByteBufVisitor --- .../bookkeeper/util/ByteBufVisitor.java | 99 ++++++++----------- 1 file changed, 39 insertions(+), 60 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index d76905285bc..0b12d7f7d53 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -51,6 +51,10 @@ public class ByteBufVisitor { private static final int DEFAULT_VISIT_MAX_DEPTH = 10; + private ByteBufVisitor() { + // prevent instantiation + } + /** * This method traverses the potential nested composite buffers of the provided buffer, given a specific offset and * length. The traversal continues until it encounters a buffer that is backed by an array or a memory address, @@ -87,11 +91,10 @@ public interface ByteBufVisitorCallback { public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, T context, int maxDepth) { InternalContext internalContext = new InternalContext<>(); - internalContext.depth = 0; internalContext.maxDepth = maxDepth; - internalContext.userContext = context; - internalContext.userCallback = callback; - recursivelyVisitBuffers(buffer, offset, length, internalContext); + internalContext.callbackContext = context; + internalContext.callback = callback; + internalContext.recursivelyVisitBuffers(buffer, offset, length); } private static class InternalContext { @@ -100,74 +103,50 @@ private static class InternalContext { ByteBuf parentBuffer; int parentOffset; int parentLength; - T userContext; - ByteBufVisitorCallback userCallback; + T callbackContext; + ByteBufVisitorCallback callback; GetBytesCallbackByteBuf callbackByteBuf = new GetBytesCallbackByteBuf(this); - InternalContextByteBufVisitorCallback callback = InternalContextByteBufVisitorCallback.get(); - } - private static class InternalContextByteBufVisitorCallback - implements ByteBufVisitorCallback> { - static final InternalContextByteBufVisitorCallback INSTANCE = new InternalContextByteBufVisitorCallback<>(); - - static InternalContextByteBufVisitorCallback get() { - @SuppressWarnings("unchecked") - InternalContextByteBufVisitorCallback instance = (InternalContextByteBufVisitorCallback) INSTANCE; - return instance; + void recursivelyVisitBuffers(ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitLength == 0) { + // skip visiting empty buffers + return; + } + // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address + // and the max depth has not been reached + if (depth < maxDepth && !visitBuffer.hasMemoryAddress() && !visitBuffer.hasArray()) { + parentBuffer = visitBuffer; + parentOffset = visitIndex; + parentLength = visitLength; + depth++; + // call getBytes to trigger the wrapped buffer visit + visitBuffer.getBytes(visitIndex, callbackByteBuf, 0, visitLength); + depth--; + } else { + passBufferToCallback(visitBuffer, visitIndex, visitLength); + } } - @Override - public void visitBuffer(InternalContext internalContext, ByteBuf visitBuffer, int visitIndex, - int visitLength) { - if (visitBuffer == internalContext.parentBuffer && visitIndex == internalContext.parentOffset - && visitLength == internalContext.parentLength) { - // visit the buffer since it was already passed to visitBuffersImpl and further recursion - // would cause unnecessary recursion up to the max depth of recursion - internalContext.userCallback.visitBuffer(internalContext.userContext, visitBuffer, - visitIndex, visitLength); + void handleBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitBuffer == parentBuffer && visitIndex == parentOffset && visitLength == parentLength) { + // further recursion would cause unnecessary recursion up to the max depth of recursion + passBufferToCallback(visitBuffer, visitIndex, visitLength); } else { // use the doRecursivelyVisitBuffers method to visit the wrapped buffer, possibly recursively - recursivelyVisitBuffers(visitBuffer, visitIndex, visitLength, internalContext); + recursivelyVisitBuffers(visitBuffer, visitIndex, visitLength); } } - @Override - public void visitArray(InternalContext internalContext, byte[] visitArray, int visitIndex, - int visitLength) { - // visit the array - internalContext.userCallback.visitArray(internalContext.userContext, visitArray, visitIndex, - visitLength); + private void passBufferToCallback(ByteBuf visitBuffer, int visitIndex, int visitLength) { + callback.visitBuffer(callbackContext, visitBuffer, visitIndex, visitLength); } - } - private static void recursivelyVisitBuffers(ByteBuf buffer, int offset, int length, - InternalContext internalContext) { - if (length == 0) { - // skip visiting empty buffers - return; - } - // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address - // and the max depth has not been reached - if (internalContext.depth < internalContext.maxDepth && !buffer.hasMemoryAddress() && !buffer.hasArray()) { - internalContext.parentBuffer = buffer; - internalContext.parentOffset = offset; - internalContext.parentLength = length; - internalContext.depth++; - visitBuffersImpl(buffer, offset, length, internalContext); - internalContext.depth--; - } else { - // visit the buffer - internalContext.userCallback.visitBuffer(internalContext.userContext, buffer, offset, length); + void handleArray(byte[] visitArray, int visitIndex, int visitLength) { + // pass array to callback + callback.visitArray(callbackContext, visitArray, visitIndex, visitLength); } } - // Implementation for visiting a single buffer level using {@link ByteBuf#getBytes(int, ByteBuf, int, int)} - private static void visitBuffersImpl(ByteBuf parentBuffer, int parentOffset, int parentLength, - InternalContext internalContext) { - // call getBytes to trigger the wrapped buffer visit - parentBuffer.getBytes(parentOffset, internalContext.callbackByteBuf, 0, parentLength); - } - /** * A ByteBuf implementation that can be used as the destination buffer for * a {@link ByteBuf#getBytes(int, ByteBuf)} for visiting the wrapped child buffers. @@ -181,13 +160,13 @@ static class GetBytesCallbackByteBuf extends ByteBuf { @Override public ByteBuf setBytes(int index, ByteBuf src, int srcIndex, int length) { - internalContext.callback.visitBuffer(internalContext, src, srcIndex, length); + internalContext.handleBuffer(src, srcIndex, length); return this; } @Override public ByteBuf setBytes(int index, byte[] src, int srcIndex, int length) { - internalContext.userCallback.visitArray(internalContext.userContext, src, srcIndex, length); + internalContext.handleArray(src, srcIndex, length); return this; } From b691316b44f2f16e24716ce29c33b29cc3b61d6c Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 22:14:32 +0200 Subject: [PATCH 35/39] Optimize the buffer copying in the case where array or memory address cannot be accessed - read-only buffers will need to be copied before reading - use ByteBuf.copy for direct buffers with pooled allocator when the algorithm can accept a memory address buffer - use the 64kB threadlocal byte[] buffer for copying all other inputs --- .../proto/checksum/CRC32CDigestManager.java | 5 ++ .../proto/checksum/CRC32DigestManager.java | 5 ++ .../proto/checksum/DigestManager.java | 40 ++++++++++------ .../proto/checksum/DummyDigestManager.java | 5 ++ .../proto/checksum/MacDigestManager.java | 5 ++ .../bookkeeper/util/ByteBufVisitor.java | 47 +++++++++++++++++-- ...ompositeByteBufUnwrapBugReproduceTest.java | 5 ++ .../circe/checksum/Crc32cIntChecksum.java | 3 ++ .../scurrilous/circe/checksum/IntHash.java | 2 + .../circe/checksum/Java8IntHash.java | 5 ++ .../circe/checksum/Java9IntHash.java | 5 ++ .../scurrilous/circe/checksum/JniIntHash.java | 5 ++ 12 files changed, 114 insertions(+), 18 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java index 0b9c469beb1..5343357198a 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java @@ -54,4 +54,9 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return Crc32cIntChecksum.resumeChecksum(digest, buffer, offset, len); } + + @Override + boolean acceptsMemoryAddressBuffer() { + return Crc32cIntChecksum.acceptsMemoryAddressBuffer(); + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java index 12d21836c9d..0d18312cfc8 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java @@ -79,4 +79,9 @@ boolean isInt32Digest() { // This is stored as 8 bytes return false; } + + @Override + boolean acceptsMemoryAddressBuffer() { + return DirectMemoryCRC32Digest.isSupported(); + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index c6d9f1abe76..01bd31832b5 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -33,7 +33,6 @@ import org.apache.bookkeeper.proto.DataFormats.LedgerMetadataFormat.DigestType; import org.apache.bookkeeper.util.ByteBufList; import org.apache.bookkeeper.util.ByteBufVisitor; -import org.apache.commons.lang3.mutable.MutableInt; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -53,7 +52,7 @@ public abstract class DigestManager { final long ledgerId; final boolean useV2Protocol; private final ByteBufAllocator allocator; - private final MutableIntByteBufVisitorCallback byteBufVisitorCallback; + private final DigestUpdaterByteBufVisitorCallback byteBufVisitorCallback; abstract int getMacCodeLength(); @@ -67,9 +66,9 @@ final int update(int digest, ByteBuf buffer, int offset, int len) { } else if (buffer.hasArray()) { return internalUpdate(digest, buffer.array(), buffer.arrayOffset() + offset, len); } else { - MutableInt digestRef = new MutableInt(digest); - ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef); - return digestRef.intValue(); + UpdateContext updateContext = new UpdateContext(digest); + ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, updateContext); + return updateContext.digest; } } @@ -84,7 +83,7 @@ public DigestManager(long ledgerId, boolean useV2Protocol, ByteBufAllocator allo this.useV2Protocol = useV2Protocol; this.macCodeLength = getMacCodeLength(); this.allocator = allocator; - this.byteBufVisitorCallback = new MutableIntByteBufVisitorCallback(); + this.byteBufVisitorCallback = new DigestUpdaterByteBufVisitorCallback(); } public static DigestManager instantiate(long ledgerId, byte[] passwd, DigestType digestType, @@ -359,26 +358,37 @@ public RecoveryData verifyDigestAndReturnLastConfirmed(ByteBuf dataReceived) thr return new RecoveryData(lastAddConfirmed, length); } - private class MutableIntByteBufVisitorCallback implements ByteBufVisitor.ByteBufVisitorCallback { + private static class UpdateContext { + int digest; + + UpdateContext(int digest) { + this.digest = digest; + } + } + + private class DigestUpdaterByteBufVisitorCallback implements ByteBufVisitor.ByteBufVisitorCallback { @Override - public void visitBuffer(MutableInt digestRef, ByteBuf visitBuffer, int visitIndex, int visitLength) { + public void visitBuffer(UpdateContext context, ByteBuf visitBuffer, int visitIndex, int visitLength) { if (visitLength > 0) { // recursively visit the sub buffer and update the digest - int updatedDigest = - internalUpdate(digestRef.intValue(), visitBuffer, visitIndex, visitLength); - digestRef.setValue(updatedDigest); + context.digest = internalUpdate(context.digest, visitBuffer, visitIndex, visitLength); } } @Override - public void visitArray(MutableInt digestRef, byte[] visitArray, int visitIndex, int visitLength) { + public void visitArray(UpdateContext context, byte[] visitArray, int visitIndex, int visitLength) { if (visitLength > 0) { // update the digest with the array - int updatedDigest = - internalUpdate(digestRef.intValue(), visitArray, visitIndex, visitLength); - digestRef.setValue(updatedDigest); + context.digest = internalUpdate(context.digest, visitArray, visitIndex, visitLength); } } + + @Override + public boolean acceptsMemoryAddress(UpdateContext context) { + return DigestManager.this.acceptsMemoryAddressBuffer(); + } } + + abstract boolean acceptsMemoryAddressBuffer(); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java index fd1f4b83c00..e2fff9bd7ca 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DummyDigestManager.java @@ -54,4 +54,9 @@ void populateValueAndReset(int digest, ByteBuf buffer) {} boolean isInt32Digest() { return false; } + + @Override + boolean acceptsMemoryAddressBuffer() { + return false; + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java index 8ec5256bdc2..f9fda5a531d 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/MacDigestManager.java @@ -111,4 +111,9 @@ int internalUpdate(int digest, byte[] buffer, int offset, int len) { boolean isInt32Digest() { return false; } + + @Override + boolean acceptsMemoryAddressBuffer() { + return false; + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 0b12d7f7d53..19a74e424ec 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -20,6 +20,7 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.util.ByteProcessor; +import io.netty.util.concurrent.FastThreadLocal; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -82,11 +83,17 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, Byte public interface ByteBufVisitorCallback { void visitBuffer(T context, ByteBuf visitBuffer, int visitIndex, int visitLength); void visitArray(T context, byte[] visitArray, int visitIndex, int visitLength); + default boolean preferArrayOrMemoryAddress(T context) { + return true; + } + default boolean acceptsMemoryAddress(T context) { + return false; + } } /** - * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback, Object)}. This method allows to specify - * the maximum depth of recursion for visiting wrapped buffers. + * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback, Object, ByteBufAllocator)}. This method + * allows to specify the maximum depth of recursion for visiting wrapped buffers. */ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, T context, int maxDepth) { @@ -97,6 +104,14 @@ public static void visitBuffers(ByteBuf buffer, int offset, int length, Byte internalContext.recursivelyVisitBuffers(buffer, offset, length); } + private static final int TL_COPY_BUFFER_SIZE = 64 * 1024; + private static final FastThreadLocal TL_COPY_BUFFER = new FastThreadLocal() { + @Override + protected byte[] initialValue() { + return new byte[TL_COPY_BUFFER_SIZE]; + } + }; + private static class InternalContext { int depth; int maxDepth; @@ -138,7 +153,33 @@ void handleBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { } private void passBufferToCallback(ByteBuf visitBuffer, int visitIndex, int visitLength) { - callback.visitBuffer(callbackContext, visitBuffer, visitIndex, visitLength); + if (callback.preferArrayOrMemoryAddress(callbackContext)) { + if (visitBuffer.hasArray()) { + handleArray(visitBuffer.array(), visitBuffer.arrayOffset() + visitIndex, visitLength); + } else if (visitBuffer.hasMemoryAddress() && callback.acceptsMemoryAddress(callbackContext)) { + callback.visitBuffer(callbackContext, visitBuffer, visitIndex, visitLength); + } else if (callback.acceptsMemoryAddress(callbackContext) && visitBuffer.isDirect() + && visitBuffer.alloc().isDirectBufferPooled()) { + // read-only buffers need to be copied before they can be directly accessed + ByteBuf copyBuffer = visitBuffer.copy(visitIndex, visitLength); + callback.visitBuffer(callbackContext, copyBuffer, 0, visitLength); + copyBuffer.release(); + } else { + // fallback to reading the visited buffer into the copy buffer in a loop + byte[] copyBuffer = TL_COPY_BUFFER.get(); + int remaining = visitLength; + int currentOffset = visitIndex; + while (remaining > 0) { + int readLen = Math.min(remaining, copyBuffer.length); + visitBuffer.getBytes(currentOffset, copyBuffer, 0, readLen); + handleArray(copyBuffer, 0, readLen); + remaining -= readLen; + currentOffset += readLen; + } + } + } else { + callback.visitBuffer(callbackContext, visitBuffer, visitIndex, visitLength); + } } void handleArray(byte[] visitArray, int visitIndex, int visitLength) { diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java index 7cd58290bf7..6252bb71be9 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java @@ -131,6 +131,11 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) { int internalUpdate(int digest, byte[] buffer, int offset, int len) { return intHash.resume(digest, buffer, offset, len); } + + @Override + boolean acceptsMemoryAddressBuffer() { + return intHash.acceptsMemoryAddressBuffer(); + } } @Test diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java index 0f0bcbe4528..d90f8b7ea5d 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java @@ -94,4 +94,7 @@ public static int resumeChecksum(int previousChecksum, byte[] payload, int offse return CRC32C_HASH.resume(previousChecksum, payload, offset, len); } + public static boolean acceptsMemoryAddressBuffer() { + return CRC32C_HASH.acceptsMemoryAddressBuffer(); + } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java index 84fb1c73364..be98ae19be1 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java @@ -30,4 +30,6 @@ public interface IntHash { int resume(int current, ByteBuf buffer, int offset, int len); int resume(int current, byte[] buffer, int offset, int len); + + boolean acceptsMemoryAddressBuffer(); } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java index bbf7d5aa6e8..2825c610b11 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java8IntHash.java @@ -58,4 +58,9 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { public int resume(int current, byte[] buffer, int offset, int len) { return hash.resume(current, buffer, offset, len); } + + @Override + public boolean acceptsMemoryAddressBuffer() { + return false; + } } diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index f615df7e069..ef8fb65a127 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -92,6 +92,11 @@ public int resume(int current, byte[] array, int offset, int length) { return ~current; } + @Override + public boolean acceptsMemoryAddressBuffer() { + return true; + } + private static int updateBytes(int current, byte[] array, int offset, int length) { try { return (int) UPDATE_BYTES.invoke(null, current, array, offset, offset + length); diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java index 7c125cca021..dc5bed0fc1c 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/JniIntHash.java @@ -56,4 +56,9 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { public int resume(int current, byte[] buffer, int offset, int len) { return hash.resume(current, buffer, offset, len); } + + @Override + public boolean acceptsMemoryAddressBuffer() { + return true; + } } From 0af155608f460b37908efaced97a778df259cd03 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 22:24:22 +0200 Subject: [PATCH 36/39] Check if memory address is accepted --- .../org/apache/bookkeeper/proto/checksum/DigestManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 01bd31832b5..3586be8ce48 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -61,7 +61,7 @@ public abstract class DigestManager { abstract int internalUpdate(int digest, byte[] buffer, int offset, int len); final int update(int digest, ByteBuf buffer, int offset, int len) { - if (buffer.hasMemoryAddress()) { + if (buffer.hasMemoryAddress() && acceptsMemoryAddressBuffer()) { return internalUpdate(digest, buffer, offset, len); } else if (buffer.hasArray()) { return internalUpdate(digest, buffer.array(), buffer.arrayOffset() + offset, len); From 317aab70ff619f2cf56c3d1b6c137e5e79b09295 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sun, 4 Feb 2024 22:31:59 +0200 Subject: [PATCH 37/39] Improve comments about complement (current = ~current) in resume --- .../bookkeeper/util/ByteBufVisitor.java | 2 +- .../circe/checksum/Java9IntHash.java | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 19a74e424ec..858b098f934 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -92,7 +92,7 @@ default boolean acceptsMemoryAddress(T context) { } /** - * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback, Object, ByteBufAllocator)}. This method + * See @{@link #visitBuffers(ByteBuf, int, int, ByteBufVisitorCallback, Object)}. This method * allows to specify the maximum depth of recursion for visiting wrapped buffers. */ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java index ef8fb65a127..2e779a92766 100644 --- a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java +++ b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java @@ -112,15 +112,14 @@ public int resume(int current, ByteBuf buffer) { @Override public int resume(int current, ByteBuf buffer, int offset, int len) { - // flip the bits (bit-wise complements) for the input value. - // this serves two purposes: - // - the CRC32C algorithm is designed to start with a seed value of all bits set to 1 (0xffffffff) - // - when 0 is passed in initially, ~0 will result in the correct initial value (0xfffffff). - // - the CRC32C algorithm is designed in a way that the final value is complemented as the last step. - // - this method will always complement the return value - // - for iterative use, the input value should be complemented to continue calculations. - // This way the algorithm can be used incrementally without a separate initialization step - // and finalization step. + // The input value is bit-wise complemented for two reasons: + // 1. The CRC32C algorithm is designed to start with a seed value where all bits are set to 1 (0xffffffff). + // When 0 is initially passed in, ~0 results in the correct initial value (0xffffffff). + // 2. The CRC32C algorithm complements the final value as the last step. This method will always complement + // the return value. Therefore, when the algorithm is used iteratively, it is necessary to complement + // the input value to continue calculations. + // This allows the algorithm to be used incrementally without needing separate initialization and + // finalization steps. current = ~current; if (buffer.hasMemoryAddress()) { @@ -141,8 +140,9 @@ public int resume(int current, ByteBuf buffer, int offset, int len) { } } - // return a complement of the current value to match the CRC32C algorithm's finalization step. - // if there's another resume step, the value will be complemented to start the next step. + // The current value is complemented to align with the finalization step of the CRC32C algorithm. + // If there is a subsequent resume step, the value will be complemented again to initiate the next step + // as described in the comments in the beginning of this method. return ~current; } } From 2c73dcae765b31b1e171520030515303433b33a8 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Thu, 1 Feb 2024 21:08:36 +0200 Subject: [PATCH 38/39] Print thread dump when build is cancelled --- .github/workflows/bk-ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/bk-ci.yml b/.github/workflows/bk-ci.yml index e8d77821147..c9f9226dd80 100644 --- a/.github/workflows/bk-ci.yml +++ b/.github/workflows/bk-ci.yml @@ -200,6 +200,10 @@ jobs: path: surefire-reports retention-days: 7 + - name: print JVM thread dumps when cancelled + if: cancelled() + run: ./dev/ci-tool print_thread_dumps + integration-tests: name: Integration Tests runs-on: ubuntu-latest From c215db82265e7175871be9f422efd0bb6087b982 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 5 Feb 2024 08:31:55 +0200 Subject: [PATCH 39/39] Filter empty buffers and arrays in ByteBufVisitor --- .../bookkeeper/proto/checksum/DigestManager.java | 12 ++++-------- .../apache/bookkeeper/util/ByteBufVisitor.java | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java index 3586be8ce48..1e78e4075eb 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java @@ -370,18 +370,14 @@ private class DigestUpdaterByteBufVisitorCallback implements ByteBufVisitor.Byte @Override public void visitBuffer(UpdateContext context, ByteBuf visitBuffer, int visitIndex, int visitLength) { - if (visitLength > 0) { - // recursively visit the sub buffer and update the digest - context.digest = internalUpdate(context.digest, visitBuffer, visitIndex, visitLength); - } + // recursively visit the sub buffer and update the digest + context.digest = internalUpdate(context.digest, visitBuffer, visitIndex, visitLength); } @Override public void visitArray(UpdateContext context, byte[] visitArray, int visitIndex, int visitLength) { - if (visitLength > 0) { - // update the digest with the array - context.digest = internalUpdate(context.digest, visitArray, visitIndex, visitLength); - } + // update the digest with the array + context.digest = internalUpdate(context.digest, visitArray, visitIndex, visitLength); } @Override diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java index 858b098f934..32e9c8c55a4 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufVisitor.java @@ -97,6 +97,10 @@ default boolean acceptsMemoryAddress(T context) { */ public static void visitBuffers(ByteBuf buffer, int offset, int length, ByteBufVisitorCallback callback, T context, int maxDepth) { + if (length == 0) { + // skip visiting empty buffers + return; + } InternalContext internalContext = new InternalContext<>(); internalContext.maxDepth = maxDepth; internalContext.callbackContext = context; @@ -123,10 +127,6 @@ private static class InternalContext { GetBytesCallbackByteBuf callbackByteBuf = new GetBytesCallbackByteBuf(this); void recursivelyVisitBuffers(ByteBuf visitBuffer, int visitIndex, int visitLength) { - if (visitLength == 0) { - // skip visiting empty buffers - return; - } // visit the wrapped buffers recursively if the buffer is not backed by an array or memory address // and the max depth has not been reached if (depth < maxDepth && !visitBuffer.hasMemoryAddress() && !visitBuffer.hasArray()) { @@ -143,6 +143,10 @@ void recursivelyVisitBuffers(ByteBuf visitBuffer, int visitIndex, int visitLengt } void handleBuffer(ByteBuf visitBuffer, int visitIndex, int visitLength) { + if (visitLength == 0) { + // skip visiting empty buffers + return; + } if (visitBuffer == parentBuffer && visitIndex == parentOffset && visitLength == parentLength) { // further recursion would cause unnecessary recursion up to the max depth of recursion passBufferToCallback(visitBuffer, visitIndex, visitLength); @@ -183,6 +187,10 @@ private void passBufferToCallback(ByteBuf visitBuffer, int visitIndex, int visit } void handleArray(byte[] visitArray, int visitIndex, int visitLength) { + if (visitLength == 0) { + // skip visiting empty arrays + return; + } // pass array to callback callback.visitArray(callbackContext, visitArray, visitIndex, visitLength); }