libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p
https://github.com/libp2p/specs/tree/master/pubsub
Other
325 stars 187 forks source link

Unclosed reader in github.com/libp2p/go-libp2p-pubsub.(*PubSub).handleNewStream #186

Open prestonvanloon opened 5 years ago

prestonvanloon commented 5 years ago

This ReaderCloser is never closed. We are seeing high memory usage on the heap from this line and I suspect it is because r.Close() is never called.

https://github.com/libp2p/go-libp2p-pubsub/blob/49274b0e8aecdf6cad59d768e5702ff00aa48488/comm.go#L33

prestonvanloon commented 5 years ago

I think the closer for this reader is just the closer of the underlying stream, but there are branches which the closer is not called.

Why not add defer r.Close() after opening the reader?

raulk commented 5 years ago

@vyzo is looking into this right now!

vyzo commented 5 years ago

The stream is properly clsoed or reset in all branches.

There is a test here: https://github.com/libp2p/go-libp2p-pubsub/blob/49274b0e8aecdf6cad59d768e5702ff00aa48488/comm.go#L38 If it is EOF then we can Close write and be done, in the other two cases the stream is Reset.

Note that defer Close() is not sufficient, as we may need to Reset in abnormal termination cases.

bruinxs commented 5 years ago

I think this part of the logic does have problems. One of my services introduced go-ipfs. It runs for a while (sometimes, sometimes two hours) and throws out of menery to exit abnormally.

I don't know why it is like this now, but I suspect it is because stream is never called.

66810012-023e1500-ef61-11e9-8298-1ffc71b27027

profile001

Stebalien commented 5 years ago

Can you pull a goroutine dump? You should see a tone of handlePeerEOF and/or handleSendingMessages goroutines.