eth: fix potential deadlock of pub/sub module #22132
Conversation
unclezoro
requested review from
holiman,
karalabe and
rjl493456442
as code owners
unclezoro
changed the title
|
Thanks for your detail issue explanation and the fix! |
| @@ -174,6 +174,17 @@ func (sub *Subscription) Unsubscribe() { | |||
| // this ensures that the manager won't use the event channel which | |||
| // will probably be closed by the client asap after this method returns. | |||
| <-sub.Err() | |||
|
|
|||
| drainLoop: | |||
There was a problem hiding this comment.
I'm not sure it's needed. After the uninstall request gets processed, no more following events will be sent to this filter. All accumulated events will be consumed by the filter handler itself.
But anwway I agree it's ugly to prevent the deadlock in the Unsubscribe by such approach.
There was a problem hiding this comment.
The uninstall request is processed in eventLoop, while eventLoop maybe busy pushing hash into sub.f.hashes, however if sub.f.hashes have unread hash, then deadlock happens.
| f.hashes = append(f.hashes, ph...) | ||
| } | ||
| api.filtersMu.Unlock() | ||
| f.hashes = append(f.hashes, ph...) |
There was a problem hiding this comment.
It may introduces some data race.
In the GetFilterChanges API will be reset the f.hashes to nil. So the lock is kind of necessary here.
There was a problem hiding this comment.
I suggest adding a fine-grained lock in filter rather than using a global lock. What do you think?
Description
Fix the potential deadlock of pub/sub module which may cause the chain halt.
More detail in #22131
Rationale
The deadlock dependency is:
Routine A:
NewPendingTransactionFilterwant to lockfiltersMuto consume hashes;Routine B:
eventLoopis waiting Routine A to consume hashes so that it can push new hash to channel.Routine C:
Unsubscribeis holding lock filtersMu, but it is waiting for Routine B to consumeuninstallchannel.Changes
There is no need to fetch lock on
NewPendingTransactionFilterroutine, so just loose the logic.And fix the potential risk that
Unsubscriberoutine does not drain all thehash,headerandlog.