From 22981f0b1d76e7a2993f96d59e6f417f737bfa4f Mon Sep 17 00:00:00 2001 From: WJL3333 Date: Sun, 16 Jul 2023 01:18:55 +0800 Subject: [PATCH 1/4] fix readBuffer leak --- .../bookkeeper/bookie/BufferedChannel.java | 5 ++++ .../bookie/BufferedReadChannel.java | 27 +++++++++++++++++++ .../bookkeeper/bookie/DefaultEntryLogger.java | 2 ++ 3 files changed, 34 insertions(+) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index 3197165827a..d746d27f068 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -101,6 +101,11 @@ public synchronized void close() throws IOException { if (closed) { return; } + + super.close(); + writeBufferStartPosition.set(0); + writeBuffer.clear(); + ReferenceCountUtil.release(writeBuffer); fileChannel.close(); closed = true; diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java index 22f5a81690d..e512905fb98 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java @@ -44,6 +44,10 @@ public class BufferedReadChannel extends BufferedChannelBase { long invocationCount = 0; long cacheHitCount = 0; + private boolean closed = false; + + private boolean isFileChannelOwner = false; + public BufferedReadChannel(FileChannel fileChannel, int readCapacity) { super(fileChannel); this.readCapacity = readCapacity; @@ -103,4 +107,27 @@ public synchronized void clear() { readBuffer.clear(); } + /** + * If the subclass is FileChannel owner it should response for close fileChannel. + * @param isOwner + */ + public void setFileChannelOwner(boolean isOwner) { + this.isFileChannelOwner = isOwner; + } + + public synchronized void close() throws IOException { + if (closed) { + return; + } + + readBufferStartPosition = Long.MIN_VALUE; + readBuffer.release(); + + if (isFileChannelOwner) { + fileChannel.close(); + } + + closed = true; + } + } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java index 64c6508d67d..96f70b5c114 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java @@ -899,6 +899,8 @@ private BufferedReadChannel getChannelForLogId(long entryLogId) throws IOExcepti // We set the position of the write buffer of this buffered channel to Long.MAX_VALUE // so that there are no overlaps with the write buffer while reading fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes()); + fc.setFileChannelOwner(true); + putInReadChannels(entryLogId, fc); return fc; } From cdd49f5dde99e7c57b70abb99dc1b5094a505e65 Mon Sep 17 00:00:00 2001 From: WJL3333 Date: Sun, 16 Jul 2023 01:23:10 +0800 Subject: [PATCH 2/4] fix readBuffer leak --- .../bookkeeper/bookie/BufferedReadChannel.java | 14 +------------- .../bookkeeper/bookie/DefaultEntryLogger.java | 1 - 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java index e512905fb98..bd0dde7d4e7 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java @@ -46,8 +46,6 @@ public class BufferedReadChannel extends BufferedChannelBase { private boolean closed = false; - private boolean isFileChannelOwner = false; - public BufferedReadChannel(FileChannel fileChannel, int readCapacity) { super(fileChannel); this.readCapacity = readCapacity; @@ -107,14 +105,6 @@ public synchronized void clear() { readBuffer.clear(); } - /** - * If the subclass is FileChannel owner it should response for close fileChannel. - * @param isOwner - */ - public void setFileChannelOwner(boolean isOwner) { - this.isFileChannelOwner = isOwner; - } - public synchronized void close() throws IOException { if (closed) { return; @@ -123,9 +113,7 @@ public synchronized void close() throws IOException { readBufferStartPosition = Long.MIN_VALUE; readBuffer.release(); - if (isFileChannelOwner) { - fileChannel.close(); - } + // BufferedReadChannel is not response for fileChannel close. closed = true; } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java index 96f70b5c114..2ecb9fcb413 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java @@ -899,7 +899,6 @@ private BufferedReadChannel getChannelForLogId(long entryLogId) throws IOExcepti // We set the position of the write buffer of this buffered channel to Long.MAX_VALUE // so that there are no overlaps with the write buffer while reading fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes()); - fc.setFileChannelOwner(true); putInReadChannels(entryLogId, fc); return fc; From 50cffc132ae5cc7230a06bdd1de571e12c399d86 Mon Sep 17 00:00:00 2001 From: wangjinlong Date: Tue, 25 Jul 2023 12:31:13 +0800 Subject: [PATCH 3/4] remove clear writeBuffer --- .../main/java/org/apache/bookkeeper/bookie/BufferedChannel.java | 1 - 1 file changed, 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index d746d27f068..55bd3e31225 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -104,7 +104,6 @@ public synchronized void close() throws IOException { super.close(); writeBufferStartPosition.set(0); - writeBuffer.clear(); ReferenceCountUtil.release(writeBuffer); fileChannel.close(); From 87c9e3fe18d8ed5903913e938b2e9ac2f5e58503 Mon Sep 17 00:00:00 2001 From: WJL3333 Date: Sat, 12 Aug 2023 14:58:11 +0800 Subject: [PATCH 4/4] use ReferenceCountUtil to release --- .../java/org/apache/bookkeeper/bookie/BufferedReadChannel.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java index bd0dde7d4e7..3ac5a22c197 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java @@ -24,6 +24,7 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; +import io.netty.util.ReferenceCountUtil; import java.io.IOException; import java.nio.channels.FileChannel; @@ -111,7 +112,7 @@ public synchronized void close() throws IOException { } readBufferStartPosition = Long.MIN_VALUE; - readBuffer.release(); + ReferenceCountUtil.release(readBuffer); // BufferedReadChannel is not response for fileChannel close.