mediasoup: Regression in 3.10.6

Yes, I know 3.10.6 is 6 months old (by the way, published on my birthday 😃) but I have been updating some old code that was using 3.9.2 and found it, and regression is still present in latest 3.11.4.

Bug Report

Your environment

  • Operating system: Ubuntu Mate 22.10 64 bits
  • Node version: v18.13.0
  • npm version: 8.19.3
  • gcc version: 12.2.0
  • mediasoup version: 3.10.6
  • mediasoup-client version: not aplicable

Issue description

My battery test is of 100% coverage, and after updating dependencies from Mediasoup 3.9.2 to 3.11.4, I found they started to fail. I’ve tried multiple versions, and since about 3.10.0 (I don’t remember the exact number) they started to be unstable, like if there were race conditions that made random exceptions to be thrown, but tests were passing since I was checking for exceptions being thrown instead of for the actual error (my fault, I have now in my todo list to both improve the tests and to take a look the source of the race conditions, since most of them are coming from out of limits Workers management, so probably I’m not doing a proper cleanup, although that didn’t happen in the past with 3.9.2, but I was also using a less powerful laptop too with less CPUs).

While testing the multiple versions, I found that at version 3.10.6 they started to fail always the same two tests, both of them related to pip’ing routers with pipeToRouter() helper function. The most simple one is:

const codec = {
  channels: 2,
  clockRate: 48000,
  mimeType: "audio/opus",
};

const routerOptions = {
  mediaCodecs: [
    {
      ...codec,
      kind: "audio",
    },
  ],
};

const router = new MafaldaRouter(createWorker, { routerOptions });

const promise = router
  .createDirectTransport()
  .then(function (transport) {
    return transport.produce(options);
  })
  .then(function (producer) {
    const router2 = new MafaldaRouter(createWorker, { routerOptions });

    return router
      .pipeToMafaldaRouter({
        producerId: producer.id,
        mafaldaRouter: router2,
      })
      .finally(router2.close.bind(router2));
  });

return expect(promise).resolves.toMatchObject({
  pipeConsumer: expect.any(Consumer),
  pipeProducer: expect.any(Producer),
});

This test from my project Mafalda is creating an instance of MafaldaRouter (an abstract wrapper on top of Mediasoup Routers that manages their creation and destruction over multiple Mediasoup Workers as if they were a single one), creating a Mediasoup DirectTransport (can be of any other type), a Producer on it, and later pip’ing that Producer to another instance of MafaldaRouter with pipeToMafaldaRouter(), that’s mostly a wrapper of Mediasoup Router.pipeToRouter() but also creating the internal Mediasoup Router object, and later destroying everything. As I’ve said, this test has been working flawlessly until upgrading Mediasoup to version 3.10.6 or newer ones, where it knows rejects with the next error:

Error: Channel request handler with ID 375d3f8c-ff9e-4f4e-94a7-c34a10961263 already exists [method:transport.produce]
          at Channel.processMessage (/home/piranna/github/Mafalda/Mafalda/node_modules/mediasoup/node/lib/Channel.js:194:37)
          at Socket.<anonymous> (/home/piranna/github/Mafalda/Mafalda/node_modules/mediasoup/node/lib/Channel.js:69:34)
          at Socket.emit (node:events:513:28)
          at addChunk (node:internal/streams/readable:324:12)
          at readableAddChunk (node:internal/streams/readable:297:9)
          at Socket.Readable.push (node:internal/streams/readable:234:10)
          at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)

It looks like someway it thinks the Producer is already in the target Router, as I was internally using the same Mediasoup Router (I’m going to check it, but have already tests to prevent that case…), but it seems more like some kind of checks or caching on the C++ side. On my own code I’ve put traces, and the error is happening exactly when internally calling to Router.pipeToRouter() method.

At version 3.10.6 there’s a lot of changes, and i will need to do git bisect to try to find when the problem started, but by eye revision, I think it could be related to https://github.com/versatica/mediasoup/commit/555e433d7cd821df53c11e624c5b47db263a3f38, or more probably to https://github.com/versatica/mediasoup/commit/39ccbc5d42c63e8cbb334a22bee5e9ed78ba0a5d, that totally makes sense to me, but it’s the kind of changes that I would put behind a major version (in fact, if that’s the culprit, I will need to go back to the design state and start over my project from scratch… 😕).

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 15 (13 by maintainers)

Most upvoted comments

As per semver rules, a breaking change would have required a MAYOR bump from 3 to 4 and that would be too much. I don’t like this behavior change being honest and I spent lot of time already trying to figure out a way to keep allowing pipeToRouter between 2 routers in same worker, but I couldn’t get it done. The problem is that, since 3.10.6, every entity (such a Producer, etc) must have a different id within the same worker, and pipeToRouter creates a new Producer with same id than the original Producer in purpose.

This is in my TODO for v4 where many other (breaking) changes will happen.

BTW I think we communicated this behavior change in Announcements in the forum, not sure, but anyway I agree it’s not nice.