Regarding the code snippet in ConcurrentBag#remove(), specifically threadLocalList.get().remove(bagEntry), when using the list obtained from threadLocalList.get() which employs weak references, removing an element of type T is an ineffective operation.

[huyu-tom]

ConcurrentBag#remove()

  public boolean remove(final T bagEntry)
   {
      if (!bagEntry.compareAndSet(STATE_IN_USE, STATE_REMOVED) && !bagEntry.compareAndSet(STATE_RESERVED, STATE_REMOVED) && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that was not borrowed or reserved: {}", bagEntry);
         return false;
      }

      final var removed = sharedList.remove(bagEntry);
      if (!removed && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that does not exist: {}", bagEntry);
      }

      threadLocalList.get().remove(bagEntry);

      return removed;
   }

It is recommended to change to the following two solutions:

1

  public boolean remove(final T bagEntry)
   {
      if (!bagEntry.compareAndSet(STATE_IN_USE, STATE_REMOVED) && !bagEntry.compareAndSet(STATE_RESERVED, STATE_REMOVED) && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that was not borrowed or reserved: {}", bagEntry);
         return false;
      }

      final var removed = sharedList.remove(bagEntry);
      if (!removed && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that does not exist: {}", bagEntry);
      }
 

     if(!useWeakThreadLocals){
       threadLocalList.get().remove(bagEntry);
     } 
      
      return removed;
   }

2

  class IWeakReference<T> extends WeakReference<T> {

  public IWeakReference(T referent) {
    super(referent);
  }

  public IWeakReference(T referent, ReferenceQueue<? super T> q) {
    super(referent, q);
  }

  @Override
  public boolean equals(Object obj) {
    return get().equals(obj);
  }
}

  public boolean remove(final T bagEntry)
   {
      if (!bagEntry.compareAndSet(STATE_IN_USE, STATE_REMOVED) && !bagEntry.compareAndSet(STATE_RESERVED, STATE_REMOVED) && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that was not borrowed or reserved: {}", bagEntry);
         return false;
      }

      final var removed = sharedList.remove(bagEntry);
      if (!removed && !closed) {
         LOGGER.warn("Attempt to remove an object from the bag that does not exist: {}", bagEntry);
      }
 
       threadLocalList.get().remove( useWeakThreadLocals ? new IWeakReference(bagEntry) : bagEntry );
      
      return removed;
   }

[brettwooldridge]

AI analysis:

The recommendations in GitHub issue #2354 regarding the ConcurrentBag#remove() method in HikariCP raise both technical and practical concerns.

Context of the Recommendations

The issue points out that when threadLocalList uses weak references, calling threadLocalList.get().remove(bagEntry) with a strong reference (bagEntry of type T) will not successfully remove the weak-encapsulated instance, leading the removal operation to be ineffective. Two alternative implementations are suggested: (1) skip the removal if weak thread locals are in use, or (2) encapsulate the target in a reference wrapper (IWeakReference) to match the stored references.

Analysis of Recommendations

Recommendation (1): Conditional Removal

• This approach avoids calling .remove() when weak references are used by checking useWeakThreadLocals before attempting to remove.
• Issue: This means objects are never removed from the threadLocalList in weak-reference mode, which can lead to stale, lingering weak references, increasing unnecessary memory use and lost tracking of what is logically present.
• Assessment: Simply skipping removal is a brittle approach that can mask underlying problems and make debugging and correct operation harder. It does not correctly handle cleanup semantics.

Recommendation (2): Wrapper-aware Removal

• Here, an IWeakReference is used, so the removal matches the type stored in the list (WeakReference<T>), allowing removal to succeed when weak references are used.
• Issue: While this would allow .remove() to operate, it introduces semantic issues:
  • The equals() method in IWeakReference only compares referent equality, which is unsafe if another WeakReference<T> was present but of a different identity, or the referent is cleared (becomes null), breaking contract expectations.
  • The collection may contain references where get() becomes null, and remove with a new wrapper will not match the original object after GC, leading to potential inconsistencies.
• Assessment: This approach improves point-removal correctness, but risks semantic confusion and potential subtle bugs if object identity or GC-timing is relied upon. It introduces custom comparison logic that may not be robust in all weak-reference scenarios.

General Assessment

Both proposals attempt to address the interface mismatch between WeakReference<T> wrappers and regular T instances, but neither fully solves the fundamental problem:

• In a weak reference context, removing by value is inherently unreliable because referents may have been cleared, and equality semantics become ambiguous.
• In Java, cleaning up weak-referenced collections typically requires reference queue management (to purge dead references), not just overriding equals().

Overall, the first proposal is overly simplistic and potentially hides real issues, while the second is only partially correct and introduces its own pitfalls. The safest, industry-standard approach for collections of weak references is active purging using reference queues and avoiding removal-by-value except in tightly controlled scenarios.

Conclusion

These recommendations are not robust or fully correct; they either oversimplify the problem or introduce unsafe custom logic. The concerns about effectiveness and correctness are well-founded: the best solution would refactor logic to leverage reference queues for cleanup and avoid value-based removal when weak references are in use.