ITADN

txnprovider/txpool: extract best's wait loop into a defer-locked helper

#23338Openyperbasis 创建于 3 天前
tech debt reduction
Y
yperbasiscommented
Follow-up to #23333; to be done after that PR merges. #23333 fixes a deadlock where `best` returned early on `ctx.Done()` while still holding `p.lock`. The fix is a point-unlock, which leaves `best` with a manually managed lock region that has several exits: - `p.lock.Lock()` at the top of the function - `p.lock.Unlock()` inside the wait loop, on the `ctx.Done()` early return (added by #23333) - `p.lock.Unlock()` after the wait loop, before `poolDB.BeginRo` (required by the two-lock ordering with the `RoTxsLimiter`, see the comment in the code) - `p.lock.Lock()` + `defer p.lock.Unlock()` for the rest of the function History shows this structure is error-prone: #16680 had to replace the original `defer`-based unlock with manual unlocks (for the RoTx ordering), and #16750 then added an early return in the wait loop and missed the unlock — exactly the deadlock #23333 fixes. The next early exit added to the loop (for example a max-wait deadline or a shutdown-flag check) would again need to hand-roll its own unlock; forgetting it compiles cleanly and passes the existing tests. ## Proposal Extract the wait loop into a helper that owns the lock for its whole scope: ```go func (p *TxPool) waitForBlock(ctx context.Context, onTopOf uint64) error { p.lock.Lock() defer p.lock.Unlock() for last := p.lastSeenBlock.Load(); last < onTopOf; last = p.lastSeenBlock.Load() { select { case <-ctx.Done(): return ctx.Err() default: // continue } p.logger.Trace("[txpool] Waiting for block", "expecting", onTopOf, "lastSeen", last, "pending", p.pending.Len(), "baseFee", p.baseFee.Len(), "queued", p.queued.Len()) p.lastSeenCond.Wait() } return nil } ``` `best` then starts with: ```go if err := p.waitForBlock(ctx, onTopOf); err != nil { return false, 0, err } tx, err := p.poolDB.BeginRo(ctx) if err != nil { return false, 0, err } defer tx.Rollback() p.lock.Lock() defer p.lock.Unlock() ``` This is behavior-identical: `Cond.Wait` re-acquires `p.lock` before it returns, so the helper's deferred unlock replaces both manual unlock sites one-for-one, and the lock is released at exactly the same points as today. Any future early exit from the wait loop gets the unlock for free. The long comment about the `RoTxsLimiter` two-lock ordering moves to the call site in `best`, in front of `poolDB.BeginRo`. The only open detail is the `txRequested` field of the trace line: pass `n` into the helper as an extra argument, or drop the field. `TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock` (added in #23333) pins the ctx-cancel path and must stay green through the refactor. Out of scope: waking a caller that is already parked in `p.lastSeenCond.Wait()` when its context is canceled (today it stays parked until the next block broadcast or shutdown) — that is a separate behavior change.
1 条评论