bitswap: clean up ledgers when disconnecting

[whyrusleeping]

License: MIT
Signed-off-by: Jeromy why@ipfs.io

[Kubuxu]

You are not locking l.lk here, and again we have situation with two locks. It shouts to me deadlock.

[whyrusleeping]

Added the ledger lock. Re locking concerns, these ones are well scoped. the engine lock is either always held first, or not held while taking the ledger lock. And the engine lock is never taken while holding a ledger lock.

[Kubuxu]

Other concern: what if PeerConnected gets the instance but can't acquire lock for ledger as it is locked by PeerDisconnected. Then PeerConnected will increase value on ledger that is not longer in ledger map.

[whyrusleeping]

@Kubuxu hrm... for that to happen, PeerConnected would have to return from findOrCreate, and then PeerDisconnected would have to take the engine lock, pull the ledger out of the map, and take the ledger lock before the PeerConnected process is able to. This IS possible.

One option is to not use findOrCreate and instead take the engine lock ourselves throughout the entire call to PeerConnected, essentially reimplementing findOrCreate in that function.

[Kubuxu]

I know that it is something that might happen very rarely or never but those edge cases add up and create hard to track down bugs and instability. If chance of this bug occuring is 0.00001% then chance that it will occur across 100000 runs is more than 60% and if we don't stop possibly introducing bugs like that go-ipfs will be always unstable and unreliable.

[whyrusleeping]

@Kubuxu Right, So i think the solution is to make PeerConnected hold the engine lock through the entire method.

[Kubuxu]

So now it is thread safe, but does function findOrCreate makes sense if we have introduced ref counting?

[Kubuxu]

Also I am still not a fan of those two locks as some not really connected change can introduce deadlock (locking for engine while holding ledger some ledger) and we might not catch it when we introduce it. We should really look into Actor oriented communication and how bad/good it will be.

[Kubuxu]

I rebased it to run coverage on it.

[Kubuxu]

It isn't tested anywhere, it might be worth to do that.

[whyrusleeping]

@Kubuxu this part of the code used to be actor oriented,and was vastly more complicated, and much more difficult to get working properly. In addition to requiring a very large amount of coroutines to get running.

…

On Fri, Dec 9, 2016, 09:46 Jakub Sztandera ***@***.***> wrote: It isn't tested anywhere, it might be worth to do that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3437 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL4HPVEbO65g29cnw4M7ZJxKLHcftjJks5rGZPtgaJpZM4K-d__> .

[Kubuxu]

I am positive that we can make it clean and not so complicated with enough layers of sugarcoating.

I am just almost sure that we will introduce deadlock around this place sooner or later and it won't be diagnosed for a long time as reproduction of this will be almost impossible.

Also for someone to report deadlock like this one he would have to 1. encounter this deadlock 2. don't try resetting the node 3. capture goroutine dump 4. have us find those blocked routines on this lock. I miss Java's features in this regard.

This change LGTM if I get some tests. In case of not directly sharness tested features I would like the codecov/patch build check to be green.

[Kubuxu]

Ok, it shows as if there was no coverage due to lack of cross package cover testing.

[ghost]

Can I add the RFM label here? Let's continue the locking discussion in #3506.