Remove the stream from internal registry upon closing

[haadcode]

Hi there 👋

This PR adds logic to remove a stream from the internal registry upon closing the stream.

As it stands now, this._streams grows unbounded upon multiple libp2p.dialProtocol(...) calls. That is, it keeps growing and keeps the old streams in this._streams.

I'm not sure if this is the right fix, nor even the right place for it given the general connection/stream/libp2p state handling, but doing this solves the problem in my project. I'd be happy to revise and work on a more correct solution.

[vasco-santos]

For several things, like the introspection protocol, I think we should keep a record of the closed streams. But, definitely we should do garbage collect at a certain point. What do you think @jacobheun ?

[jacobheun]

We already clean the streams up, introspection will poll for data and can infer removal so I don't think there's any need to keep closed streams around.

There is probably a different bug here, likely with streams not ending properly.

In the onEnd handler we call registry.delete(id). This change adds _removeStream, which is redundant because the registry is mapped to the pertinent registry.

The likely reason this fixes the problem downstream is the registry clear added here:

js-libp2p-mplex/src/mplex.js

Lines 221 to 225 in f9de8bd

	case MessageTypes.CLOSE_INITIATOR:
	case MessageTypes.CLOSE_RECEIVER:
	stream.close()
	this._removeStream(id)
	break

When we receive an mplex close message we remove the stream from tracking before it's actually closed, as streams can be half closed. It would be good to look at tests that check the stream registries and see if those are detailed enough to catch stream end issues.

[haadcode]

stream end issues

Great insights! This is pretty much what I've been seeing also: one side of the stream ends correctly (goes into the onSourceEnd/onSinkEnd handlers, can't remember where exactly they were) but the other end not, so they're I suppose "half closed" as you mention. And that's actually why I ended up fixing it like in the PR as I wasn't able to figure out how to fix the stream closing part correctly. Any pointers where to look or what could be happening and what should happen?

Fwiw, I'm observing these issues using Websockets as the transport. I have a "client-server" protocol and on both ends I call await stream.close() when the workload is done and the stream can be closed from application perspective, but that doesn't seem to do the trick.

[jacobheun]

@haadcode I've got 2 PRs open for a related issue that I think should help resolve this, as streams were definitely not being closed properly.

#116
libp2p/js-libp2p#787

As a note await stream.close() doesn't currently behave properly, it only closes half the stream right now. I've started a breaking change PR at #115 to fix this and to also introduce closeRead and closeWrite methods for finer control.

[lidel]

Closing this in favour of #115