Re: [PR] [improve][misc][WIP] Detect "double release" and "use after release" bugs with recycled objects [pulsar]

["onobc (via GitHub)" <gi...@apache.org>]

onobc commented on code in PR #22110:
URL: https://github.com/apache/pulsar/pull/22110#discussion_r1518970434


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java:
##########
@@ -47,56 +47,57 @@ protected EntryImpl newObject(Handle<EntryImpl> handle) {
     private Runnable onDeallocate;
 
     public static EntryImpl create(LedgerEntry ledgerEntry) {
-        EntryImpl entry = RECYCLER.get();
+        EntryImpl entry = getEntryFromRecycler();
         entry.timestamp = System.nanoTime();
         entry.ledgerId = ledgerEntry.getLedgerId();
         entry.entryId = ledgerEntry.getEntryId();
         entry.data = ledgerEntry.getEntryBuffer();
         entry.data.retain();
-        entry.setRefCnt(1);
+        return entry;
+    }
+
+    private static EntryImpl getEntryFromRecycler() {

Review Comment:
   👍🏻 factoring this out makes it easier to follow (and add other common behaviors in one place later if needed - basic DRY stuff). 



##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/AbstractValidatingReferenceCounted.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.pulsar.common.util;
+
+import io.netty.util.AbstractReferenceCounted;
+import io.netty.util.IllegalReferenceCountException;
+import io.netty.util.Recycler;
+
+public abstract class AbstractValidatingReferenceCounted extends AbstractReferenceCounted {
+    private static final boolean refCountCheckOnAccess =
+            Boolean.parseBoolean(System.getProperty("pulsar.refcount.check.on_access", "true"));
+
+    /**
+     * Validate that the instance hasn't been released before accessing fields.
+     * This is a sanity check to ensure that we don't read fields from deallocated objects.
+     */
+    protected void checkRefCount() {
+        if (refCountCheckOnAccess && refCnt() < 1) {
+            throw new IllegalReferenceCountException(
+                    "Possible double release bug (refCnt=" + refCnt() + "). " + getClass().getSimpleName()
+                            + " has been deallocated. ");
+        }
+    }
+
+    public final void resetRefCnt() {
+        setRefCnt(1);

Review Comment:
   It is unfortunate that `setRefCnt` is protected. Otherwise, this could have been done in a single util rather than inserted into the hierarchy (composition over inheritance). Also, the code in both hierarchies is almost identical.
   
   - The checkOnAccess var is just a static lookup.
   - The checkRefCount and  getFromRecycler could be passed in the counter or recycler, respectively
   
   Not the end of the world but my brain is having trouble leaving it alone. 



##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/AbstractValidatingReferenceCounted.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.pulsar.common.util;
+
+import io.netty.util.AbstractReferenceCounted;
+import io.netty.util.IllegalReferenceCountException;
+import io.netty.util.Recycler;
+
+public abstract class AbstractValidatingReferenceCounted extends AbstractReferenceCounted {
+    private static final boolean refCountCheckOnAccess =
+            Boolean.parseBoolean(System.getProperty("pulsar.refcount.check.on_access", "true"));
+
+    /**
+     * Validate that the instance hasn't been released before accessing fields.
+     * This is a sanity check to ensure that we don't read fields from deallocated objects.
+     */
+    protected void checkRefCount() {
+        if (refCountCheckOnAccess && refCnt() < 1) {
+            throw new IllegalReferenceCountException(
+                    "Possible double release bug (refCnt=" + refCnt() + "). " + getClass().getSimpleName()
+                            + " has been deallocated. ");
+        }
+    }
+
+    public final void resetRefCnt() {
+        setRefCnt(1);
+    }
+
+    public static <T extends AbstractValidatingReferenceCounted> T getAndCheck(Recycler<T> recycler) {

Review Comment:
   **[NIT]** It is not really doing a "check". In the ACRC it was called `getEntryFromRecycler`. I think it would be helpful to name them the same thing (sans "entry"), maybe `getInstanceFromRecycler`.



##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -1374,8 +1375,10 @@ protected ProducerImpl.ChunkedMessageCtx newObject(
                 };
 
         public static ChunkedMessageCtx get(int totalChunks) {
-            ChunkedMessageCtx chunkedMessageCtx = RECYCLER.get();
-            chunkedMessageCtx.setRefCnt(totalChunks);
+            ChunkedMessageCtx chunkedMessageCtx = getAndCheck(RECYCLER);

Review Comment:
   Is setting ref count to 1 and then retaining N-1 less performant than setting ref count to N? 
   
   **OR**
   
   Is setting ref count to 1 and then retaining N-1 more functionally correct than setting ref count to N?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org