-
Notifications
You must be signed in to change notification settings - Fork 382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG]a dead lock case #170
Comments
Dose |
data.worker still been blocked on
|
Another deadlock:
|
Thanks for reporting! This needs some additional thought. Seems like the culprit here is not the EventBus but rather the TorrentWorker's synchronized methods (onPeerDiscovered and onPeerDisconnected in the cases that you've encountered, with onPeerDisconnected being the only method that is used by the pool cleaner). I'm not sure why these methods were marked synchronized at the moment they were added (61abc06) -- most probably to enforce the hard limit on the number of peers -- but there's a good chance that things that they do can be done in a different way that would not need lock- or monitor-based synchronization. And onPeerDisconnected method in particular looks like it does not need to be synchronized at all... |
Yes, both EventBus and TorrentWorker have this issue. |
I'm not 100% sure, but to me it looks like the problem is with
TorrentWorker.onPeerDisconnected, because the deadlock involves the pool
cleaner. The fact that the EventBus is synchronized only has the effect
that events are processed one at a time, effectively turning it into a
synchronous blocking queue with guaranteed order of event ids, which is
probably intended. I'm not saying that this is a good design or that it
can't lead to problems in other places, but the one that you're running
into should be solved by making TorrentWorker.onPeerDisconnected not
synchronized. It will create a race condition, but the limit on the number
of peers should still hold.
…On Fri, 16 Jul 2021, 04:46 maomao, ***@***.***> wrote:
Yes, both EventBus and TorrentWorker have this issue.
Since EventBus also has synchronized methods(firePeerDiscovered and
firePeerConnected and firePeerDisconnected) that same with
TorrentWorker(onPeerDiscovered and onPeerDisconnected and
onPeerDisconnected)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#170 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4TJBTQ6HSHXWUM35WNPJTTX6MPBANCNFSM5AJNF4YA>
.
|
EventBus has 8 fireXXX methods, but only the 3 fireXXX methods marked as synchronized. |
Ok, I will look into it and your PR in more detail in the nearest time
…On Fri, 16 Jul 2021, 10:15 maomao, ***@***.***> wrote:
EventBus has 8 fireXXX methods, but only the 3 fireXXX methods marked as
synchronized.
From git history, the 3 methods is original first 3 methods, and other 5
was added by flowing commits.
That is, events are processed one at a time is not 100% guaranteed.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#170 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4TJBQL5YBTCLXRQUDDEVDTX7TAXANCNFSM5AJNF4YA>
.
|
Please also see the above pull request too. It uses lockfree algorithms to remove synchronization from TorrentWorker while ensuring that the limits set in the configuration are exactly adhered to. |
Update comment, fix more race conditions related to Bug: #170
Describe the bug
A dead lock occured. And then cause download blocked.
Dead lock reason:
cleaner thread acquired
connectionLock
then try acquiringEventBus Lock
collector thread acquired
EventBus Lock
then try acquiringconnectionLock
stack of ConnectionPool Cleaner thread
stack of peer-collector thread
The text was updated successfully, but these errors were encountered: