libs/CList: automatically detach the prev/next elements in Remove function (#6626)

closes #2207

Also added `Clear()` function if we want to clear whole elements in CList.

CHANGELOG_PENDING.md

@@ -20,6 +20,7 @@ Friendly reminder: We have a [bug bounty program](https://hackerone.com/tendermi
   - [cli] \#6372 Introduce `BootstrapPeers` as part of the new p2p stack. Peers to be connected on  startup (@cmwaters)
   - [config] \#6462 Move `PrivValidator` configuration out of `BaseConfig` into its own section. (@tychoish)
   - [rpc] \#6610 Add MaxPeerBlockHeight into /status rpc call (@JayT106)
+  - [libs/CList] \#6626 Automatically detach the prev/next elements in Remove function (@JayT106)
 
 - Apps
   - [ABCI] \#6408 Change the `key` and `value` fields from `[]byte` to `string` in the `EventAttribute` type. (@alexanderbez)

internal/evidence/pool.go

@@ -489,12 +489,14 @@ func (evpool *Pool) batchExpiredPendingEvidence(batch dbm.Batch) (int64, time.Ti
 func (evpool *Pool) removeEvidenceFromList(
 	blockEvidenceMap map[string]struct{}) {
 
-	for e := evpool.evidenceList.Front(); e != nil; e = e.Next() {
+	el := evpool.evidenceList
+	var nextE *clist.CElement
+	for e := el.Front(); e != nil; e = nextE {
+		nextE = e.Next()
 		// Remove from clist
 		ev := e.Value.(types.Evidence)
 		if _, ok := blockEvidenceMap[evMapKey(ev)]; ok {
 			evpool.evidenceList.Remove(e)
-			e.DetachPrev()
 		}
 	}
 }

internal/libs/clist/clist.go

@@ -196,21 +196,26 @@ func (e *CElement) SetPrev(newPrev *CElement) {
 	e.mtx.Unlock()
 }
 
-func (e *CElement) SetRemoved() {
+// Detach is a shortcut to mark the given element remove and detach the prev/next elements.
+func (e *CElement) Detach() {
 	e.mtx.Lock()
+	defer e.mtx.Unlock()
 
 	e.removed = true
 
-	// This wakes up anyone waiting in either direction.
 	if e.prev == nil {
 		e.prevWg.Done()
 		close(e.prevWaitCh)
+	} else {
+		e.prev = nil
 	}
+
 	if e.next == nil {
 		e.nextWg.Done()
 		close(e.nextWaitCh)
+	} else {
+		e.next = nil
 	}
-	e.mtx.Unlock()
 }
 
 //--------------------------------------------------------------------------------
@@ -348,24 +353,26 @@ func (l *CList) PushBack(v interface{}) *CElement {
 	return e
 }
 
-// CONTRACT: Caller must call e.DetachPrev() and/or e.DetachNext() to avoid memory leaks.
+// Remove removes the given element in the CList
 // NOTE: As per the contract of CList, removed elements cannot be added back.
+// Because CList detachse the prev/next element when it removes the given element,
+// please do not use CElement.Next() in the for loop postcondition, uses
+// a variable ahead the for loop and then assigns the Next() element
+// to it in the loop as the postcondition.
 func (l *CList) Remove(e *CElement) interface{} {
 	l.mtx.Lock()
+	defer l.mtx.Unlock()
 
 	prev := e.Prev()
 	next := e.Next()
 
 	if l.head == nil || l.tail == nil {
-		l.mtx.Unlock()
 		panic("Remove(e) on empty CList")
 	}
 	if prev == nil && l.head != e {
-		l.mtx.Unlock()
 		panic("Remove(e) with false head")
 	}
 	if next == nil && l.tail != e {
-		l.mtx.Unlock()
 		panic("Remove(e) with false tail")
 	}
 
@@ -390,13 +397,53 @@ func (l *CList) Remove(e *CElement) interface{} {
 		next.SetPrev(prev)
 	}
 
-	// Set .Done() on e, otherwise waiters will wait forever.
-	e.SetRemoved()
+	e.Detach()
 
-	l.mtx.Unlock()
 	return e.Value
 }
 
+// Clear removes all the elements in the CList
+func (l *CList) Clear() {
+	l.mtx.Lock()
+	defer l.mtx.Unlock()
+
+	if l.head == nil || l.tail == nil {
+		return
+	}
+
+	for e := l.head; e != nil; e = l.head {
+		prevE := e.Prev()
+		nextE := e.Next()
+
+		if prevE == nil && e != l.head {
+			panic("CList.Clear failed due to nil prev element")
+		}
+
+		if nextE == nil && e != l.tail {
+			panic("CList.Clear failed due to nil next element")
+		}
+
+		if l.len == 1 {
+			l.wg = waitGroup1()
+			l.waitCh = make(chan struct{})
+		}
+		l.len--
+
+		if prevE == nil {
+			l.head = nextE
+		} else {
+			prevE.SetNext(nextE)
+		}
+		if nextE == nil {
+			l.tail = prevE
+		} else {
+			nextE.SetPrev(prevE)
+		}
+
+		e.Detach()
+	}
+}
+
 func waitGroup1() (wg *sync.WaitGroup) {
 	wg = &sync.WaitGroup{}
 	wg.Add(1)

internal/libs/clist/clist_test.go

@@ -252,9 +252,16 @@ func TestScanRightDeleteRandom(t *testing.T) {
 	// time.Sleep(time.Second * 1)
 
 	// And remove all the elements.
-	for el := l.Front(); el != nil; el = el.Next() {
+	// we detach the prev/next element in CList.Remove, so just assign l.Front direcly.
+	halfElements := numElements / 2
+	for el := l.Front(); el != nil && halfElements > 0; el = l.Front() {
 		l.Remove(el)
+		halfElements--
 	}
+
+	// remove the rest half elements in the CList
+	l.Clear()
+
 	if l.Len() != 0 {
 		t.Fatal("Failed to remove all elements from CList")
 	}

internal/mempool/v0/clist_mempool.go

@@ -164,10 +164,7 @@ func (mem *CListMempool) Flush() {
 	_ = atomic.SwapInt64(&mem.txsBytes, 0)
 	mem.cache.Reset()
 
-	for e := mem.txs.Front(); e != nil; e = e.Next() {
-		mem.txs.Remove(e)
-		e.DetachPrev()
-	}
+	mem.txs.Clear()
 
 	mem.txsMap.Range(func(key, _ interface{}) bool {
 		mem.txsMap.Delete(key)
@@ -338,7 +335,6 @@ func (mem *CListMempool) addTx(memTx *mempoolTx) {
 // 	- resCbRecheck (lock not held) if tx was invalidated
 func (mem *CListMempool) removeTx(tx types.Tx, elem *clist.CElement, removeFromCache bool) {
 	mem.txs.Remove(elem)
-	elem.DetachPrev()
 	mem.txsMap.Delete(mempool.TxKey(tx))
 	atomic.AddInt64(&mem.txsBytes, int64(-len(tx)))
 

internal/mempool/v1/mempool.go

@@ -305,7 +305,6 @@ func (txmp *TxMempool) Flush() {
 			txmp.txStore.RemoveTx(wtx)
 			txmp.priorityIndex.RemoveTx(wtx)
 			txmp.gossipIndex.Remove(wtx.gossipEl)
-			wtx.gossipEl.DetachPrev()
 		}
 	}
 
@@ -743,7 +742,6 @@ func (txmp *TxMempool) removeTx(wtx *WrappedTx, removeFromCache bool) {
 	// Remove the transaction from the gossip index and cleanup the linked-list
 	// element so it can be garbage collected.
 	txmp.gossipIndex.Remove(wtx.gossipEl)
-	wtx.gossipEl.DetachPrev()
 
 	atomic.AddInt64(&txmp.sizeBytes, int64(-wtx.Size()))