Should StreamMuxerBox track how many "active" substreams are still around? #2865

Activity

1. thomaseizinger

   added

   help wanted

   on Sep 2, 2022
2. thomaseizinger commented on Sep 2, 2022

   

   thomaseizinger

   on Sep 2, 2022

   ContributorAuthor

   More actions

   Tagging as "help wanted" because I'd like to get people's input on this idea.
3. thomaseizinger

   changed the title [-]RFC: Have `StreamMuxerBox` track how many "active" substreams are still around[/-] [+]Should `StreamMuxerBox` track how many "active" substreams are still around?[/+] on Sep 2, 2022
4. mxinden commented on Sep 6, 2022

   

   mxinden

   on Sep 6, 2022

   Member

   More actions

   I like the idea. Though instead of using the Arc::weak_count to enforce a limit, I suggest using it to build a Prometheus metric. I would imagine a Connection emits an event whenever a new stream is created. That event would as well contain the overall count of streams on that connection.

   I am against using Arc::weak_count as a limit as I find it unintuitive. We pass ownership of the stream to the ConnectionHandler implementation, thus I think it is the ConnectionHandler that has to manage the resource impact.

   👍React with 👍1Reacted by Alexander Shishenko
5. thomaseizinger commented on Sep 6, 2022

   

   thomaseizinger

   on Sep 6, 2022

   ContributorAuthor

   More actions

   I like the idea. Though instead of using the Arc::weak_count to enforce a limit, I suggest using it to build a Prometheus metric. I would imagine a Connection emits an event whenever a new stream is created. That event would as well contain the overall count of streams on that connection.

   I am against using Arc::weak_count as a limit as I find it unintuitive. We pass ownership of the stream to the ConnectionHandler implementation, thus I think it is the ConnectionHandler that has to manage the resource impact.

   The issue I see with that is that we don't know, when a stream is de-allocated unless we use some kind of shared, atomic counter that is decremented on Drop. Once we pass the stream to the ConnectionHandler, we lose control over it.
6. mxinden commented on Sep 8, 2022

   

   mxinden

   on Sep 8, 2022

   Member

   More actions

   I think my comment was misleading @thomaseizinger. Let me rephrase:

   I am against using Arc::weak_count as a limit as I find it unintuitive.

   I am against using Arc::weak_count as a limit. I am in favor of using Arc::weak_count for metrics.

   Sorry. Is the second attempt more clear?
7. thomaseizinger commented on Sep 8, 2022

   

   thomaseizinger

   on Sep 8, 2022

   Last edited by thomaseizinger

   ContributorAuthor

   More actions

   Sort of. I wouldn't know how to enforce a limit via Arc::weak_count?

   What I am proposing in this issue is to use expose functions on StreamMuxerBox:

   impl StreamMuxerBox {
   	pub fn active_inbound_streams(&self) -> usize;
   	pub fn active_outbound_streams(&self) -> usize;
   }

   The implementation will return Arc::weak_count of an Arc that is embedded in every SubstreamBox and thus happen transparently for each muxer. What clients like swarm::Connection do with this information is up to them and not further defined in this issue. We can of course implement a metric on top of that.

   The motivation for this issue was to use it in swarm::Connection as the guard for calling StreamMuxer::poll_inbound which should be more robust than the current implementation which only enforces how many substreams are concurrently upgrading. If a protocol does not perform its IO in the InboundUpgrade trait but passes the substream to the ConnectionHandler, this limit is by-passed.
8. thomaseizinger commented on Sep 8, 2022

   

   thomaseizinger

   on Sep 8, 2022

   ContributorAuthor

   More actions

   Draft implementation here: #2878

   I noticed that we are unnecessarily exposing the SubstreamBox::new API publicly which is currently used in a test in libp2p-swarm. That test changes with #2861 though so I am planning to remove the SubstreamBox::new API in order to not unnecessarily expose the implementation detail of tracking active streams via Arcs.