Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChannelMessageHandlers: Make RegisterHandler() not remove the existing handler if another one with same id is given #952

Merged
merged 7 commits into from
Nov 6, 2022

Commits on Nov 6, 2022

  1. Add extra pipeToRouter() test

    Add a test to assert than calling `router1.pipeToRouter(router2)` fails if both `Routers` are in the same `Worker`.
    
    ### TODO
    
    The new test succeed however it **breaks** an unrelated test that runs later that should NOT be affected at all:
    
    ```
    FAIL  node/tests/test-PipeTransport.js
     ✓ router.pipeToRouter() succeeds with audio (24 ms)
     ✓ router.pipeToRouter() succeeds with video (21 ms)
     ### NEW TEST:
     ✓ router.pipeToRouter() fails if both Routers belong to the same Worker (32 ms)
     ✓ router.createPipeTransport() with enableRtx succeeds (6 ms)
     ✓ router.createPipeTransport() with invalid srtpParameters must fail (3 ms)
     ✓ router.createPipeTransport() with enableSrtp succeeds (9 ms)
     ✓ router.createPipeTransport() with fixed port succeeds (8 ms)
     ✓ transport.consume() for a pipe Producer succeeds (6 ms)
     ### UNRELATED FAILING TEST:
     ✕ producer.pause() and producer.resume() are transmitted to pipe Consumer (2 ms)
     ✓ producer.close() is transmitted to pipe Consumer (2 ms)
     ✓ router.pipeToRouter() succeeds with data (7 ms)
     ✓ transport.dataConsume() for a pipe DataProducer succeeds (2 ms)
     ✓ dataProducer.close() is transmitted to pipe DataConsumer (2 ms)
     ✓ router.pipeToRouter() called twice generates a single PipeTransport pair (14 ms)
     ✓ router.pipeToRouter() called in two Routers passing one to each other as argument generates a single a single PipeTransport pair (10 ms)
    
     ● producer.pause() and producer.resume() are transmitted to pipe Consumer
    
       Channel request handler with ID c9cba815-1128-4500-aa4f-7edbc135b889 not found [method:producer.resume]
    ```
    
    It doesn't make sense that such a test fails with that error (producer not found in the worker).
    
    To simplify the error, I've added a temporal code in the new test that clearly shows the issue:
    
    ```ts
    test('router.pipeToRouter() fails if both Routers belong to the same Worker', async () =>
    {
    	const router1bis = await worker1.createRouter({ mediaCodecs });
    
    	await expect(router1.pipeToRouter(
    		{
    			producerId : videoProducer.id,
    			router     : router1bis
    		}))
    		.rejects
    		.toThrow(Error);
    
    	// TODO: Temporal code to show an unexpected error.
    	{
    		// This is ok.
    		expect(videoProducer.closed).toBe(false);
    
    		// However these will fail since the handler Id does not exist in the worker.
    		// This is impossible and must be a bug.
    		await videoProducer.resume();
    		await videoProducer.pause();
    	}
    
    	router1bis.close();
    }, 2000);
    ```
    
    In addition, Rust test is missing but I don't want to deal with that until the issue is solved.
    ibc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    7c5978d View commit details
    Browse the repository at this point in the history
  2. forgot to add this code

    ibc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    0c8acd1 View commit details
    Browse the repository at this point in the history
  3. Remove temporal work

    ibc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    9b9bb29 View commit details
    Browse the repository at this point in the history
  4. ChannelMessageHandlers::RegisterHandler(): do NOT remove the existing…

    … handler id another one with same id is given
    ibc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    9039a4a View commit details
    Browse the repository at this point in the history
  5. Update CHANGELOG

    ibc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    e262b5b View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    93b5413 View commit details
    Browse the repository at this point in the history
  7. Fix Rust test

    nazar-pc committed Nov 6, 2022
    Configuration menu
    Copy the full SHA
    433c1a5 View commit details
    Browse the repository at this point in the history