pipeToRouter(): Use static map of PipeTransport pairs

[ibc]

• If router1.pipeToRouter(router2) is called and later router2.pipeToRouter(router1) is called, then the latter will reuse the already existing pair of PipeTransports instead of creating a new pair.

[ibc]

This is not ready yet. I'm adding a test and, in fact, it was buggy.

[ibc]

Ready

[nazar-pc]

I really dislike global state like this.

What I would suggest to do instead is to replace PipeTransport[] with Promise<PipeTransport[]> and set key in maps of both routers involved. And similarly when one router is closed to remove corresponding entry on all routers that are piped to/from it.

I can't say I know how AwaitQueue works, but above will probably allow to remove its need too.

[ibc]

Honestly I don't see any benefits in your approach. This is indeed a global state because it must take into account all existing Routers no matter in which Worker they have been created. When calling router1.pipeToRoute(router2) the method must access a global state in which it may find that there is a PipeTransport pair involving both router1 and router2, no matter in which Router it was previously created. And such a global state must be somewhere, and I prefer to keep it private in Router class than adding a public instance setter in Router to set the pair externally. In summary, I consider that this is a perfect use case for global state.

[nazar-pc]

When calling router1.pipeToRoute(router2) the method must access a global state

In my approach it only needs to access internal state of both routers, which it can, and no global state is needed, just internal states of those 2 routers. I can implement this when I have some time. Global state is better to be avoided and for this use cases it totally can be avoided.

[piranna]

Must to say, i've just did an implementation between two servers that's almost equal to previous code (with this fixed and other improvements), and was thinking that maybe pipeToRouter could be moved out to an external mediasoup-helpers package, so Mediasoup Code can be reduced... so maybe using internal info is not so good :-/

[ibc]

@nazar-pc I've another reason to go the way this PR is done. This:

// AwaitQueue instance to make pipeToRouter tasks happen sequentially.
private static readonly pipeToRouterQueue =
	new AwaitQueue({ ClosedErrorClass: InvalidStateError });

This queue must be global, why? because the app may call router1.pipeToRoute(route2) and router2.pipeToRoute(route1) at the same time, and that would lead to inconsistent state in which the PipeTransport pair is generated twice (instead of just once and replicated in both routers as you suggest). Why? Because the first call to router1.pipeToRoute(route2) is async and it takes some time until completed. If router2.pipeToRoute(route1) is called in the meantime, two things may happen:

1. Two PipeTransport pairs are generated in both routers, meaning that the latter will override the former, etc (problems).
2. Or, depending on the implementation, the second call would find an already PipeTransport pair that is not yet completed and, when trying to use it, it would fail.

And both cases are wrong.

[ibc]

In the other side, I agree pipeToRouter() is sugar API that probably should not be included in Router API, but we cannot change this within v3.X.X.

[piranna]

Obviously It can't be removed in a 3.x version, but could be moved to the mediasoup-helpers package and use It as dependency. I have seen some other APIs with a similar state, let me review them and open a new issue listing them.

[ibc]

but could be moved to the mediasoup-helpers package and use It as dependency

It's very clear to me that such a mediasoup-helper package should not be included within mediasoup itself. Otherwise it means lot of sugar API and opinionated features to maintain as part of the core library (since it would be a direct dependency).

[nazar-pc]

would lead to inconsistent state

In my case it would not, this is why I suggested to store mapping to Promise<PipeTransport[]> from the very beginning 😉

You can store the promise before it is resolved in a map. Thus avoiding mentioned issue and allowing any number of pipe transports to be created concurrently (in your case even with multiple workers involved, all of them will wait on each other).

[piranna]

It's very clear to me that such a mediasoup-helper package should not be included within mediasoup itself. Otherwise it means lot of sugar API and opinionated features to maintain as part of the core library (since it would be a direct dependency).

Good point, my sugestion about using It as a dependency was only while in the 3.x branch to maintain backward compatibility, once that a future 4.x version appears that would be removed and users pointed to use mediasoup-helpers directly if they want, making core mediasoup smaller.

[ibc]

In my case it would not, this is why I suggested to store mapping to Promise<PipeTransport[]> from the very beginning 😉

Oh yes... right! I'll do.

BTW will you be able to replicate it in Rust? I've very limited time these days and learning Rust doesn't fit well.

[nazar-pc]

BTW will you be able to replicate it in Rust? I've very limited time these days and learning Rust doesn't fit well.

Yes, should be.

[ibc]

my sugestion about using It as a dependency was only while in the 3.x branch to maintain backward compatibility, once that a future 4.x version appears that would be removed and users pointed to use mediasoup-helpers directly if they want, making core mediasoup smaller.

Being honest I don't want to maintain any mediasoup-helper package, and it wouldn't make sense that it just provides with the pipeToRoute() helper. So I prefer to not create any separate library nor include it in v3.

[nazar-pc]

I believe pipeToRouter() is very valuable for a simple, but very common use case of using multiple CPU cores/workers on the same server. Yes, you can do the same thing without it, but it is cumbersome to manage and pipeToRouter() makes it a breathe. And on Rust side there are also clever type system tricks applied that make much nicer than you could do with wrappers. I don't see why it needs to be moved out into helper library.

[ibc]

@nazar-pc please, re-review, you'll like it :)

(and I've removed the awaitqueue dependency).

[nazar-pc]

I like it much better indeed, still a few comments

[ibc]

@nazar-pc re-review please :)

[piranna]

Being honest I don't want to maintain any mediasoup-helper package, and it wouldn't make sense that it just provides with the pipeToRoute() helper.

I've put a description of the things that I've seen can be moved out at #705.