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

Use single thread for usrsctp stack #2383

Closed
wants to merge 2 commits into from
Closed

Use single thread for usrsctp stack #2383

wants to merge 2 commits into from

Conversation

lminiero
Copy link
Member

@lminiero lminiero commented Oct 1, 2020

Yesterday we noticed that the usrsctp stack always created, and bound to, two random UDP ports: one for IPv4 and one for IPv6. Digging in the code, we found out the cause was the usrsctp_init(0, janus_sctp_data_to_dtls, NULL) call: as part of the initialization process, the usrsctp stack creates a thread and binds to the provided port (random one, in our case, since we pass 0) in order to support SCTP over UDP encapsulation.

Clearly we don't need that kind of encapsulation in Janus: actually, we don't want it at all! But apparently the only way to get rid of it is starting the usrsctp stack without its internal threads, which means driving it using an external loop. This is a feature that was added last year, thanks to the efforts by @jmillan and @ibc that revived an older contribution that had been discarded at the time. More specifically, when you initialize the usrsctp stack with usrsctp_init_nothreads instead of usrsctp_init, no threads are created in the stack, which means the UDP encapsulation stuff isn't created either. Of course, it also means as an application you're responsible for "running" the stack somehow, which in the context of usrsctp means running the usrsctp_handle_timers() timer as part of your event loop.

This patch tries to do exactly that, by adding support for this nothreads mode. By default we still behave as before (usrsctp_init called at startup), but if you set usrsctp_singlethread = true in the general section of janus.jcfg, we switch to the single thread mode instead: this means initializing the stack with usrsctp_init_nothreads, and creating a dedicated thread in sctp.c to host a Glib context/loop that can then serve the regular source that will invoke usrsctp_handle_timers (which we do every 10ms, at the moment, as José Luis and Iñaki did in mediasoup). I considered re-using one of the existing loops for that timer, like the main loop in the Janus core, but I don't have a clear understanding of the impact the usrsctp stack may have on loops yet, and so I thought it safer to have a dedicated thread instead.

From a few simple tests this seems to be working, but this needs proper testing, especially with higher numbers and a higher datachannel traffic (which is why in this PR it's optional and disabled by default). Feedback welcome!

@atoppi
Copy link
Member

atoppi commented Oct 1, 2020

IMHO if the purpose of the patch is just getting rid of the allocated UDP ports, I think it is not ideal to disable the internal usrsctp loop, thus forcing Janus to handle it.
I'd rather support a patch on usrsctp that can disable the UDP encapsulation without dealing with its threading model.

@ibc
Copy link

ibc commented Oct 1, 2020

In mediasoup we run every C++ worker into a single libuv loop, including usrsctp in/out, so I'd say having usrsctp running into a shared loop doesn't have side effects (it never blocks, etc).

BTW do you plan to report the unsolicited UDP port in usrsctp project? The fact that it's not created in nothreads mode is just a "fortunate" side effect, but IMHO it's a clear bug.

@lminiero
Copy link
Member Author

lminiero commented Oct 1, 2020

BTW do you plan to report the unsolicited UDP port in usrsctp project? The fact that it's not created in nothreads mode is just a "fortunate" side effect, but IMHO it's a clear bug.

I've prepared a PR with a way to start with no UDP encapsulation even when using threads right now, explaining why we consider it an issue. I'll give you a link as soon as it's ready.

@ibc
Copy link

ibc commented Oct 1, 2020

Awesoming

@lminiero
Copy link
Member Author

lminiero commented Oct 1, 2020

Created here: sctplab/usrsctp#532

@lminiero
Copy link
Member Author

lminiero commented Oct 1, 2020

After some useful tips by @tuexen, we updated the instructions in the README on how to install usrsctp to disable those features we don't need. As such, this PR isn't needed anymore and we can close it.

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.

3 participants