txnprovider/txpool: extract best's wait loop into a defer-locked helper
tech debt reduction
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 条评论