fix: reference counting (retain/release) in PerChannelBookieClient #4293

lhotari · 2024-04-16T10:41:44Z

Motivation

This addresses the remaining gaps of #4289 in handling ByteBuf retain/release.
This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: #791 (comment) .

This PR fixes several problems:

ByteString buffer lifecycle in client, follows ByteBufList lifecycle
ByteBufList lifecycle, moved to write promise
Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises.

Changes

add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite
- use these callback actions for proper cleanup
extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString
properly handle releasing of ByteBufList in the write promise
properly handle calling promises that are buffered while authentication is in progress

lhotari · 2024-04-22T14:46:41Z

@hangc0276 @poorbarcode Please review. This addresses the remaining gaps of #4289.

lhotari · 2024-04-26T11:33:01Z

@hangc0276 @poorbarcode @codelipenghui @BewareMyPower @merlimat Please review. This addresses the remaining gaps of #4289.

dlg99

LGTM, just a small comment

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java

lhotari · 2024-05-23T19:22:52Z

I finally found the root cause in Bookkeeper client. The client had several buffer leaks since the ByteBufList lifecycle was incorrectly handled and write promises were dropped and ignored when authentication was in progress.
@merlimat @hangc0276 @poorbarcode @eolivelli @nicoloboschi @dlg99 Please review!
I hope we could release 4.16.6 after this PR is processed, merged and cherry-picked to branch-4.16 .

shoothzj · 2024-05-23T22:34:35Z

Can we close #3902 after this pr is merged?

hangc0276 · 2024-05-24T01:17:48Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java

+                            ChannelPromise promise;
+                            // check if the message has an associated promise as the next element in the queue
+                            if (waitingForAuth.peek() instanceof ChannelPromise) {
+                                promise = (ChannelPromise) waitingForAuth.poll();


If found this issue before https://github.com/apache/bookkeeper/pull/3902/files. Your solution is better.

hangc0276 · 2024-05-24T01:54:47Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

-            body = UnsafeByteOperations.unsafeWrap(toSend.toArray());
-        }
+        ByteString body = ByteStringUtil.byteBufListToByteString(toSend);
+        toSend.retain();


The toSend already convert to ByteString, why do we need to retain and release after flushed no matter the flush succeed or not?

The ByteString doesn't handle the lifecycle of the buffers referenced by the ByteString.
It's the toSend.retain() which handles the lifecycle.
The cleanupActionFailedBeforeWrite and cleanupActionAfterWrite parameters (Runnables) are used to ensure that the reference count is properly decreased later.

We already converted toSend to ByteString, the lifecycle of toSend can be tracked by the caller of the method.

hangc0276 · 2024-05-24T01:59:10Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

-                }
-            }
+            ByteString body = ByteStringUtil.byteBufListToByteString(bufToSend);
+            bufToSend.retain();


Do not need retain the bufToSend?

same answer as #4293 (comment)

hangc0276 · 2024-05-24T02:07:30Z

Nice catch!

…4293) ### Motivation This addresses the remaining gaps of #4289 in handling ByteBuf retain/release. This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: #791 (comment) . This PR fixes several problems: * ByteString buffer lifecycle in client, follows ByteBufList lifecycle * ByteBufList lifecycle, moved to write promise * Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises. ### Changes - add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite - use these callback actions for proper cleanup - extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString - properly handle releasing of ByteBufList in the write promise - properly handle calling promises that are buffered while authentication is in progress (cherry picked from commit 0ef2f99)

hangc0276 · 2024-05-26T00:05:03Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ByteBufList.java

+                ChannelPromise compositePromise = ctx.newPromise();
+                compositePromise.addListener(future -> {
+                    // release the ByteBufList after the write operation is completed
+                    ReferenceCountUtil.safeRelease(b);


Use ReferenceCountUtil.relesse(b) to expose potential exceptions in release the ByteBufList?

…pache#4293) This addresses the remaining gaps of apache#4289 in handling ByteBuf retain/release. This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: apache#791 (comment) . This PR fixes several problems: * ByteString buffer lifecycle in client, follows ByteBufList lifecycle * ByteBufList lifecycle, moved to write promise * Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises. - add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite - use these callback actions for proper cleanup - extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString - properly handle releasing of ByteBufList in the write promise - properly handle calling promises that are buffered while authentication is in progress

…pache#4293) ### Motivation This addresses the remaining gaps of apache#4289 in handling ByteBuf retain/release. This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: apache#791 (comment) . This PR fixes several problems: * ByteString buffer lifecycle in client, follows ByteBufList lifecycle * ByteBufList lifecycle, moved to write promise * Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises. ### Changes - add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite - use these callback actions for proper cleanup - extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString - properly handle releasing of ByteBufList in the write promise - properly handle calling promises that are buffered while authentication is in progress (cherry picked from commit 0ef2f99) # Conflicts: # bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

…4293) (#4396) ### Motivation This addresses the remaining gaps of #4289 in handling ByteBuf retain/release. This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: #791 (comment) . This PR fixes several problems: * ByteString buffer lifecycle in client, follows ByteBufList lifecycle * ByteBufList lifecycle, moved to write promise * Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises. ### Changes - add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite - use these callback actions for proper cleanup - extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString - properly handle releasing of ByteBufList in the write promise - properly handle calling promises that are buffered while authentication is in progress (cherry picked from commit 0ef2f99) # Conflicts: # bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

lhotari mentioned this pull request Apr 16, 2024

[fix] Fix ByteBuf release/retain in PerChannelBookClient #4289

Merged

lhotari force-pushed the lh-fix-reference-counts-in-PerChannelBookieClient branch from 37fe66d to 98bc499 Compare April 16, 2024 10:55

lhotari changed the title ~~fix: reference counts in PerChannelBookieClient~~ fix: reference counting (retain/release) in PerChannelBookieClient Apr 16, 2024

lhotari force-pushed the lh-fix-reference-counts-in-PerChannelBookieClient branch 10 times, most recently from 1a7542d to 90f9325 Compare April 16, 2024 12:05

lhotari mentioned this pull request Apr 16, 2024

Bookkeeper client allocates a lot of short-lived objects #791

Closed

lhotari force-pushed the lh-fix-reference-counts-in-PerChannelBookieClient branch from 90f9325 to 77b09c2 Compare April 22, 2024 14:41

lhotari mentioned this pull request Apr 26, 2024

[Bug] parseMessageMetadata error when broker entry metadata enable with high loading apache/pulsar#22601

Open

3 tasks

nicoloboschi approved these changes Apr 30, 2024

View reviewed changes

shoothzj approved these changes Apr 30, 2024

View reviewed changes

dlg99 approved these changes Apr 30, 2024

View reviewed changes

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java Show resolved Hide resolved

shoothzj added release/4.16.6 release/4.17.1 labels May 16, 2024

lhotari mentioned this pull request May 23, 2024

[fix][broker] Fix multiple transfer corruption issues when TLS is enabled apache/pulsar#22760

Closed

4 tasks

lhotari added 6 commits May 23, 2024 14:20

fix: reference counts in PerChannelBookieClient

b62ba86

Extract helper class

230271c

Add solution to ByteBufList.Encoder to release after the write completes

59dd9cd

Remove unnecessary clone of ByteBufList

d05e976

Fix bug in ByteStringUtil

fd5431a

Add ByteStringUtil optimization

6dfe88d

Add handling for channel promises to AuthHandler

74f9782

lhotari force-pushed the lh-fix-reference-counts-in-PerChannelBookieClient branch from 77b09c2 to 74f9782 Compare May 23, 2024 19:12

Fix ByteBufListTest

b6127bf

merlimat approved these changes May 23, 2024

View reviewed changes

poorbarcode approved these changes May 24, 2024

View reviewed changes

hangc0276 reviewed May 24, 2024

View reviewed changes

hangc0276 approved these changes May 24, 2024

View reviewed changes

codelipenghui approved these changes May 24, 2024

View reviewed changes

shoothzj merged commit 0ef2f99 into apache:master May 24, 2024
21 checks passed

shoothzj mentioned this pull request May 24, 2024

Fix promise lost in ClientSide AuthHandler #3902

Closed

lhotari mentioned this pull request May 24, 2024

[improve][misc][WIP] Detect "double release" and "use after release" bugs with recycled objects apache/pulsar#22110

Draft

4 tasks

shoothzj added the cherry-picked/branch-4.17 label May 25, 2024

hangc0276 reviewed May 26, 2024

View reviewed changes

lhotari mentioned this pull request May 27, 2024

[branch-4.16] fix: reference counting (retain/release) in PerChannelBookieClient (#4293) #4396

Merged

shoothzj added the cherry-picked/branch-4.16 label May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reference counting (retain/release) in PerChannelBookieClient #4293

fix: reference counting (retain/release) in PerChannelBookieClient #4293

lhotari commented Apr 16, 2024 •

edited

lhotari commented Apr 22, 2024

lhotari commented Apr 26, 2024

dlg99 left a comment

lhotari commented May 23, 2024

shoothzj commented May 23, 2024

hangc0276 May 24, 2024

hangc0276 May 24, 2024

lhotari May 24, 2024

hangc0276 May 26, 2024

hangc0276 May 24, 2024

lhotari May 24, 2024

hangc0276 commented May 24, 2024

hangc0276 May 26, 2024

fix: reference counting (retain/release) in PerChannelBookieClient #4293

fix: reference counting (retain/release) in PerChannelBookieClient #4293

Conversation

lhotari commented Apr 16, 2024 • edited

Motivation

Changes

lhotari commented Apr 22, 2024

lhotari commented Apr 26, 2024

dlg99 left a comment

Choose a reason for hiding this comment

lhotari commented May 23, 2024

shoothzj commented May 23, 2024

hangc0276 May 24, 2024

Choose a reason for hiding this comment

hangc0276 May 24, 2024

Choose a reason for hiding this comment

lhotari May 24, 2024

Choose a reason for hiding this comment

hangc0276 May 26, 2024

Choose a reason for hiding this comment

hangc0276 May 24, 2024

Choose a reason for hiding this comment

lhotari May 24, 2024

Choose a reason for hiding this comment

hangc0276 commented May 24, 2024

hangc0276 May 26, 2024

Choose a reason for hiding this comment

lhotari commented Apr 16, 2024 •

edited