-
Notifications
You must be signed in to change notification settings - Fork 98
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
erldns_udp_server: Fix flow control #134
base: main
Are you sure you want to change the base?
Conversation
Reset the {active, N} socket option only when receiving an 'udp_passive' message, rather than on every received packet, which provides no flow control (and therefore no advantage over {active, true}).
I should mention that I didn't actually test this change, sorry about that. I just stumbled over the code while looking into various UDP servers written in Erlang (I was trying to figure out sane UDP socket options for a TURN server). |
Thanks, @weiss for opening the PR. I've checked the documentation and this does seem like a good approach. We will run some tests before we accept your contribution. |
@weiss - have you managed to test this thoroughly and performance benchmarks to compare against current implementation? If you have done this, may you please share it with us? |
@m0rcq I didn't test this, sorry (I don't use erldns myself). However, I guess this might be a misunderstanding: My commit is meant to be a bug fix, not an "enhancement" of some way. For context,
Your code is enabling If you really want that behavior (for max. performance), you'd just specify |
@weiss - thanks a lot for the thorough explanation, appreciated! We are discussing internally how to allocate a bit of time to benchmark this and verify erldns' behaviour under load with the suggested fix. Stay tuned, we will update the ticket once we have something to share. |
Reset the
{active, N}
socket option only when receiving anudp_passive
message, rather than on every received packet, which provides no flow control and therefore no advantage over{active, true}
mode. (BTW, resetting{active, N}
actually addsN
to the current count, so adding100
on every packet could in theory overflow the counter at some point.)