fix memory leak holding onto streams unnecessarily #93
Conversation
whyrusleeping
force-pushed
the
fix/strmap-leak
branch
from
6e81395
to
5b5c3ab
Compare
|
please review carefully the |
dht_net.go
Outdated
| dht.smlk.Unlock() | ||
|
|
||
| ms = &messageSender{p: p, dht: dht} | ||
| if err := ms.prep(); err != nil { |
There was a problem hiding this comment.
Can't we just make a single messageSender and synchronize on its lock for prep?
There was a problem hiding this comment.
we cant do that without holding the dht.smlk for the duration of the prep call. And the prep call may take arbitrarily long (up to a dial timeout). We don't want to block every peer from calling this message waiting on a dial. :/
There was a problem hiding this comment.
We can sync on the lock outside the smlk.
I was thinking of starting with something like this:
ms = &messageSender{...}
dht.strmap[p] = ms
dht.smlk.Unlock()
ms.lk.Lock()
if err := ms.prep(); ...
There was a problem hiding this comment.
We can even acquire the edit: that would block.ms.lk inside smlk
There was a problem hiding this comment.
Obviously, the subsequent logic would have to be adjusted, but i think it might result in simpler code.
There was a problem hiding this comment.
Then what happens when two separate goroutines have the same message sender (one created it and one got it from the map and is waiting on the prep lock) and then prep fails? goroutine that called prep exits, and the goroutine that just got a reference to the messageSender and was waiting on the lock now tries to re-call prep, which is okay if prep fails again, but if prep succeeds on that second call for some reason, then we have an open stream attached to a message sender that isnt being tracked by anything.
There was a problem hiding this comment.
yeah, this is the problem. I had a similar issue in #92 and resolved it with a reset count, but it's hairy.
| @@ -133,13 +164,10 @@ type messageSender struct { | |||
| p peer.ID | |||
| dht *IpfsDHT | |||
|
|
|||
| invalid bool | |||
There was a problem hiding this comment.
what is the purpose of this? doesn't seem to be used.
There was a problem hiding this comment.
Now it is. We were toying with using it and decided we didn't need it but forgot to remove it. Now, we realized we really do need it.
If you can think of a better, race-free way of doing this dance, suggestions welcome!
Stebalien
force-pushed
the
fix/strmap-leak
branch
from
5b5c3ab
to
4237ef0
Compare
dht_net.go
Outdated
| singleMes int | ||
| } | ||
|
|
||
| func (dht *IpfsDHT) newMessageSender(p peer.ID) *messageSender { | ||
| return &messageSender{p: p, dht: dht} | ||
| // invalidate is called after this messageSender is removed form the strmap. |
There was a problem hiding this comment.
This is not true, it is called before.
This was definitely causing a decent memory leak. I'm sorry.