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

feat: add callback for unhandled STUN requests #1211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jun 14, 2024

Calls the functions added to libjuice in paullouisageneau/libjuice#248 (this needs to be merged & released before CI will pass here).

Exports a OnUnhandledStunRequest function that can be passed a callback that will be invoked when an incoming STUN message is received that has no corresponding agent for the ICE ufrag.

Refs #1166

Calls the functions added to libjuice in paullouisageneau/libjuice#248

Exports a `OnUnhandledStunRequest` function that can be passed a
callback that will be invoked when an incoming STUN message is
received that has no corresponding agent for the ICE ufrag.

Closes paullouisageneau#1166
@achingbrain
Copy link
Contributor Author

achingbrain commented Jun 14, 2024

When implementing this I hadn't realised that libjuice will only ever allow one UDP socket to be used so unregistering the handler takes a host/port pair in order to unregister the handler only for that pair.

I hope this can be worked around as it's quite common to create two listeners during a test run and they'll not be able to co-exist in the same process like this.

We could, for example invalidate the module cache when loading the native part of node-datachannel to get a separate copy of libdatachannel that would run in it's own memory space but it's a bit of a hack.

Also if the application has already created a peer connection outside of libp2p's control, setting the host/port will have no effect as it would have already opened the port specified in the first invocation, and now we'd be setting the handler for that port which may not be what the user expects. Clarified by @xicilion below

Anyway, if you'd prefer a separate function without the host/port to remove the handler please shout.

It may not need the void *user_ptr arg to juice_bind_stun then since it can just invoke the global reference stored in unboundStunCallback.

@xicilion
Copy link
Contributor

Also if the application has already created a peer connection outside of libp2p's control, setting the host/port will have no effect as it would have already opened the port specified in the first invocation, and now we'd be setting the handler for that port which may not be what the user expects.

This should not happen. In order to avoid such hidden errors, I added a check in juice_bind_stun to prevent binding when mux connections are detected, so as to avoid binding to ports that users do not expect.

@xicilion
Copy link
Contributor

The Mux mode is bound to a unique socket, which has no problem at all in a normal WebRTC application, after all, you can't even set the port in the browser.

But in the Peer B application of WebRTC Direct, it is restricted by this. One process can only bind to one port, which greatly limits its usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants