TransportListenInfo: Add portRange (deprecate worker port range) #1365

ibc · 2024-04-07T20:39:11Z

Details

New optional portRange field (an object with min and max keys) has been added in TransportListenInfo, so it's available in router.createWebRtcTransport(), router.createPlainTransport(), router.createPipeTransport() and worker.createWebRtcServer() methods.
If portRange is given, then a free random port in that range (and given ip and protocol) will be chosen.
This feature deprecates worker ports range (rtcMinPort and rtcMaxPort in WorkerSettings given to createWorker() method). Rationale is that it doesn't make any sense to have a unique port range for different listening IPs (for example: an app may want to use different port ranges for public connections (public internet) and private connections (private network).

TODO

Documentation in website and proper announcement.

ibc · 2024-04-07T23:43:55Z

node/src/Channel.ts

@@ -1,5 +1,6 @@
 import * as os from 'node:os';
 import { Duplex } from 'node:stream';
+import { info, warn } from 'node:console';


This is to avoid that Jest mangles the output of MS_DUMP().

ibc · 2024-04-07T23:46:24Z

node/src/test/test-WebRtcTransport.ts

+test('router.createWebRtcTransport() with fixed port succeeds', async () => {
+	const port = await pickPort({
+		type: 'tcp',
+		ip: '127.0.0.1',
+		reserveTimeout: 0,
+	});
+	const webRtcTransport = await ctx.router!.createWebRtcTransport({
+		listenInfos: [
+			// NOTE: udpReusePort flag will be ignored since protocol is TCP.
+			{ protocol: 'tcp', ip: '127.0.0.1', port, flags: { udpReusePort: true } },
+		],
+	});
+
+	expect(webRtcTransport.iceCandidates[0].port).toEqual(port);
+
+	webRtcTransport.close();
+}, 2000);


Moved to keep a proper order.

ibc · 2024-04-07T23:46:53Z

worker/include/RTC/PortManager.hpp

@@ -14,50 +13,72 @@ namespace RTC
 	class PortManager
 	{
 	private:
-		enum class Transport : uint8_t
+		enum class Protocol : uint8_t


consistency

ibc · 2024-04-07T23:47:52Z

worker/include/RTC/TransportTuple.hpp

-		 |IP[0] ^  IP[1] ^ IP[2] ^ IP[3] |          IP[0] >> 16      |F|P|
-		 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-		 */
-		void SetHash()


Moved to .cpp

ibc · 2024-04-07T23:50:16Z

worker/src/RTC/UdpSocket.cpp

 	  : // This may throw.
-	    ::UdpSocketHandle::UdpSocketHandle(PortManager::BindUdp(ip, port, flags)), listener(listener),
-	    fixedPort(true)


We had a critical BUG here! We marked UdpSocket with fixedPort: true, meaning that the port was never removed from PortManager's maps when this UdpSocket was closed!!!

ibc · 2024-04-07T23:52:11Z

worker/src/RTC/UdpSocket.cpp

+	  uint16_t minPort,
+	  uint16_t maxPort,
+	  RTC::Transport::SocketFlags& flags,
+	  uint64_t& portRangeHash)


Note that portRangeHash is passed as a reference to uint64_t, so the parent constructor (in the initializer list) calls RTC::PortManager::BindUdp() and also passes portRangeHash to it as a reference to uint64_t, so PortManager will change the value of portRangeHash and the new value will be also visible here.

jmillan

Nice 👍 . If I had a comment to add is that it would be IMHO a bit cleaner adding a new type portRange rather than two minPort and maxPort.

type PortRange = {
  min: number;  
  max: number;
};

ibc · 2024-04-08T10:31:19Z

rust/src/data_structures.rs

@@ -52,7 +52,7 @@ impl AppData {
 /// # Notes on usage
 /// If you use "0.0.0.0" or "::" as ip value, then you need to also provide
 /// `announced_address`.
-#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, Deserialize, Serialize)]


I had to remove Ord and PartialOrd because RangeInclusive doesn't implement them. No downsides AFAIS.

rust/src/data_structures.rs

ibc · 2024-04-08T10:32:33Z

@jmillan, minPort and maxPort now moved to portRange.

CosmosisT · 2024-04-10T21:29:40Z

Why not allowedPorts and provide ability for min/max and static ports. That way everyone is settled. I would assume if isolated processes are ran the min/max range would still be affective skipping used ports for an available one when initiating connections.

The parameter would look a little like this allowedPorts: {min: 20000, max: 30000, static:[443, 3923, 10293]}

This way no matter the setup it's simplified and in one spot and for people who want specifics between TCP/UDP connections this would suffice.

ibc · 2024-04-10T22:23:09Z

We allow passing a static port (so the app must manage ports by itself) or a port range. IMHO is more than enough. In addition the PortRange C++ class is optimized to avoid trying already used ports.

Also, it doesn't make sense that the app passes some static ports plus a port range. If the app passes a static port then the app is supposed to manage ports. mediasoup is not gonna try first your "maybe available or not" static ports and then, of all fail, choose a random one in the given range. It doesn't make sense.

ibc added 10 commits April 6, 2024 13:06

more

f182b4c

cosmetic

f5298c3

cosmetic

3ea785f

done

87ea07f

Update CHANGELOG

7667885

fix cargo clippy errors

5ab1bbe

fix rust assert! usage

93bc30c

cosmetic

5c48e0b

fix hash and cosmetic

6ece7ac

fix lint

b647806

ibc marked this pull request as ready for review April 7, 2024 23:35

ibc requested a review from jmillan April 7, 2024 23:41

ibc commented Apr 7, 2024

View reviewed changes

cosmetic

8f2c057

ibc commented Apr 7, 2024

View reviewed changes

ibc changed the title ~~ListenInfo: Add minPort and maxPort (deprecate worker port range)~~ TransportListenInfo: Add minPort and maxPort (deprecate worker port range) Apr 7, 2024

jmillan approved these changes Apr 8, 2024

View reviewed changes

Use portRange instead of minPort and maxPort

7ec9b6c

ibc changed the title ~~TransportListenInfo: Add minPort and maxPort (deprecate worker port range)~~ TransportListenInfo: Add portRange (deprecate worker port range) Apr 8, 2024

ibc added 2 commits April 8, 2024 12:29

Update CHANGELOG

bef7876

cosmetic

87679e2

ibc commented Apr 8, 2024

View reviewed changes

rust/src/data_structures.rs Outdated Show resolved Hide resolved

ibc requested a review from nazar-pc April 8, 2024 10:35

cosmetic

7248b72

ibc added 2 commits April 8, 2024 13:19

make it Rust idiomatic (ehem...)

f77cea1

Merge branch 'v3' into dynamic-port-ranges

14ee81b

ibc merged commit 62d2731 into v3 Apr 8, 2024
31 checks passed

ibc deleted the dynamic-port-ranges branch April 8, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransportListenInfo: Add portRange (deprecate worker port range) #1365

TransportListenInfo: Add portRange (deprecate worker port range) #1365

ibc commented Apr 7, 2024 •

edited

Loading

ibc Apr 7, 2024

ibc Apr 7, 2024

ibc Apr 7, 2024

ibc Apr 7, 2024

ibc Apr 7, 2024

ibc Apr 7, 2024

jmillan left a comment

ibc Apr 8, 2024

ibc commented Apr 8, 2024

CosmosisT commented Apr 10, 2024 •

edited

Loading

ibc commented Apr 10, 2024

TransportListenInfo: Add portRange (deprecate worker port range) #1365

TransportListenInfo: Add portRange (deprecate worker port range) #1365

Conversation

ibc commented Apr 7, 2024 • edited Loading

Details

TODO

ibc Apr 7, 2024

Choose a reason for hiding this comment

ibc Apr 7, 2024

Choose a reason for hiding this comment

ibc Apr 7, 2024

Choose a reason for hiding this comment

ibc Apr 7, 2024

Choose a reason for hiding this comment

ibc Apr 7, 2024

Choose a reason for hiding this comment

ibc Apr 7, 2024

Choose a reason for hiding this comment

jmillan left a comment

Choose a reason for hiding this comment

ibc Apr 8, 2024

Choose a reason for hiding this comment

ibc commented Apr 8, 2024

CosmosisT commented Apr 10, 2024 • edited Loading

ibc commented Apr 10, 2024

ibc commented Apr 7, 2024 •

edited

Loading

CosmosisT commented Apr 10, 2024 •

edited

Loading