diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 76fc81819..57ac36421 100644 --- a/CHANGELOG_PENDING.md +++ b/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) diff --git a/internal/evidence/pool.go b/internal/evidence/pool.go index 8ca97fd17..643ff9e7d 100644 --- a/internal/evidence/pool.go +++ b/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() } } } diff --git a/internal/libs/clist/clist.go b/internal/libs/clist/clist.go index 6cf515706..64b5cecc9 100644 --- a/internal/libs/clist/clist.go +++ b/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) diff --git a/internal/libs/clist/clist_test.go b/internal/libs/clist/clist_test.go index 4b222e3d5..9a997e5dc 100644 --- a/internal/libs/clist/clist_test.go +++ b/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") } diff --git a/internal/mempool/v0/clist_mempool.go b/internal/mempool/v0/clist_mempool.go index 40e93cc13..636426267 100644 --- a/internal/mempool/v0/clist_mempool.go +++ b/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))) diff --git a/internal/mempool/v1/mempool.go b/internal/mempool/v1/mempool.go index 9b06ee4a8..afaa88322 100644 --- a/internal/mempool/v1/mempool.go +++ b/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()))