paullouisageneau / libjuice

JUICE is a UDP Interactive Connectivity Establishment library
Mozilla Public License 2.0
403 stars 75 forks source link

support juice_bind_stun to reflect stun requests from unbound client. #248

Open xicilion opened 3 months ago

xicilion commented 3 months ago

In order to implement WebRTC Direct PeerB, we need to capture STUN requests from unknown clients and return the client's information. Once PeerB receives this information, it will construct the client's SDP and establish a connection with the client on its own.

xicilion commented 2 months ago

change argument at callback function.

xicilion commented 2 months ago

This should be okay, but I need to do some testing to make sure the socket will be closed automatically. I'll add it.

xicilion commented 2 months ago

It turned out to be a little more complicated than I expected, but I did it.

achingbrain commented 2 months ago

Awesome!

Is the new juice_unbind_stun() function going to unbind all listeners and ports?

I'm thinking of the case where a user specifies multiple listeners running on different ports - they should be stoppable independently of each other.

xicilion commented 2 months ago

I discussed this with @paullouisageneau . Currently, Juice actually only supports one port for mux. After the first mux connects to the specified port, all other mux connections will use this port. So Juice will only have one mux socket at a time.

https://github.com/paullouisageneau/libjuice/issues/247

achingbrain commented 2 months ago

So Juice will only have one mux socket at a time

This will prevent people doing things like opening multiple WebRTC-Direct listeners or running more than one js-libp2p node in the same process since they'll try to use the same UDP port for incoming connections.

These aren't very common deployment patterns, though it's not unheard of, and it's done quite often while running unit/integration/interop tests.

It would be good to fix this, though probably not in this PR.

xicilion commented 2 months ago

It would be good to fix this

I also hope to achieve this refactoring, but that might be a big project. In the end, it's basically a combination of con_mux and conn_poll.

achingbrain commented 1 month ago

@paullouisageneau do you have any feedback on this PR or the functionality it adds?

paullouisageneau commented 1 month ago

This will prevent people doing things like opening multiple WebRTC-Direct listeners or running more than one js-libp2p node in the same process since they'll try to use the same UDP port for incoming connections.

These aren't very common deployment patterns, though it's not unheard of, and it's done quite often while running unit/integration/interop tests.

It would be good to fix this, though probably not in this PR.

This is not just a fix, as it affects how the API for this feature is designed. I think it needs to be accounted for, if not implemented, sooner than later. You have to design the interface in accordance now because you can't break compatibility later. This is native library packaged in a few distributions and releasing a new major version is complicated.

xicilion commented 1 month ago

@paullouisageneau Thank you very much for your feedback, I will make the changes according to your suggestions as soon as possible.

xicilion commented 1 month ago

Do you have any more suggestions for this pull request, @paullouisageneau?

achingbrain commented 1 month ago

@xicilion this PR still only allows a single muxed port per process, or am I reading it wrong?

@paullouisageneau requested it support multiple ports - https://github.com/paullouisageneau/libjuice/pull/248#discussion_r1641106178

xicilion commented 1 month ago

To support multiple ports, we'll need to almost rewrite the implementation of conn_mux.c. Currently, juice_mux_listen and juice_mux_stop_listen are already compatible with supporting multiple ports in the future. I think this time we can start by only adding the listen feature.

SgtPooki commented 9 hours ago

Is anything else needed here? I'm looking forward to seeing this completed so we can unblock https://github.com/libp2p/js-libp2p/pull/2583