eth/filters: fix potential deadlock in filter timeout loop #22178
Conversation
s1na
requested review from
holiman,
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
s1na
force-pushed
the
txfilter-deadlock
branch
from
42d0b53
to
c12bc5a
Compare
|
|
||
| // Wait until filters have timed out | ||
| time.Sleep(250 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
We should also check that all created filters have actually timed out.
There was a problem hiding this comment.
Ah on second thought that comment is misleading. At least one filter will time out but we won't know for sure exactly how many will, because during the unsubscription of one of them the routine will hang.
Should I check to see if at least one has been uninstalled (by checking api.filters length?)
There was a problem hiding this comment.
If the deadlock does not happen, all filters should be gone after this sleep. We should check this because it's an invariant here. You can check this by running GetFilterChanges for all filters (you'll need to collect their IDs into a slice for that). The GetFilterChanges call should return an error for all of them.
There was a problem hiding this comment.
Makes sense. Added the check in the select clause's happy case.
There was a problem hiding this comment.
Why though? We can always check this, right? Worst case it will produce another error message from the test. It's OK to raise more than one error from a test.
There was a problem hiding this comment.
You're right I don't know why my brain couldn't process a test failing twice :)
I just tried this but there's another problem. when a deadlock happens GetFilterChanges also hangs. One option I can think of is to go unsafe and directly read len(api.filters) without taking the lock. Wdyt?
eth/filters: fix typo in comment eth/filters: make test less order and clock dependent eth/filters: minor fixes
s1na
force-pushed
the
txfilter-deadlock
branch
from
c12bc5a
to
a9eb593
Compare
|
Updated with a possible fix. By the way this issue also affects the block and log filters. |
s1na
changed the title
fjl
changed the title
fjl
changed the title
fjl
changed the title
…22178) This fixes ethereum#22131 and adds a test reproducing the issue.
This PR adds a test reproducing #22131. It fails reliably on my machine but I could increase the tx bombardment time range to increase the odds of hitting the bug.