Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BP-61: Revert BP-59 to release ByteBuf using ReferenceCountUtil.release() instead of ReferenceCountUtil.safeRelease() #3792

Closed
hangc0276 opened this issue Feb 21, 2023 · 0 comments · Fixed by #3796

Comments

@hangc0276
Copy link
Contributor

hangc0276 commented Feb 21, 2023

BP

Follow the instructions at https://bookkeeper.apache.org/community/bookkeeper-proposals/ to create a proposal.

This is the master ticket for tracking BP-61 :

Motivation

In BP-59, which was not discussed in the dev mail list and without a vote refactored the ByteBuf release method by ReferenceCountUtil.safeRelease() instead of ByteBuf.release().

In the ReferenceCountUtil.safeRelease(), it catches the release exception. This change can make the ByteBuf release without any exceptions and makes the code run well, but it will make bugs hide deeper and more challenging to find out.

public static void safeRelease(Object msg) {
     try {
         release(msg);
     } catch (Throwable t) {
         logger.warn("Failed to release a message: {}", msg, t);
     }
 }

For example, if there is a bug to release the ByteBuf twice, whose refcnt is 1, we can find out the bug quickly if we use ByteBuf.release(), but the bug will be hard to find out if we use ReferenceCountUtil.safeRelease()

There are 12 PRs to refactor the ByteBuf release method, and I suggest reverting those PRs.

Considering some PRs have been cherry-picked into the release branch but others not, it will be hard to cherry-pick if we just push one PR to revert all the above PRs. I will push three Prs for master, branch-4.15, and branch-4.14 to apply this change.

Proposal PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment