-
-
Notifications
You must be signed in to change notification settings - Fork 332
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
acceptor-conns_sup pairs #198
Conversation
Last night on IRC:
So I think we will start work on master starting from this branch (after review of course) and then see what more improvements we can add. The important part is getting the API right as soon as possible, the internals can be changed afterwards, even after releasing 2.0, if for example we find a single process to work better than separate acceptor/sup. /cc @petrohi |
So basically you're saying to brush off the dust this PR acquired since I put it up (ie, resolve the merge conflicts), so it can be properly reviewed, yes? :) |
I'm saying I will review it. There's no hurry though, I probably won't have time before next week anyway. |
Yeah, same here. In theory, I have plenty of time since I'm on vacation this week, but family has already claimed it all, so there =^^= |
fyi https://stressgrid.com/blog/100k_cps_with_elixir/ - they use the PR and subsequently SO_REUSEPORT to push past the SYN bottleneck.. |
Yep mentioned it in #110. |
I will begin work on getting this merged next week. |
src/ranch.erl
Outdated
[_, _, _, Protocol, _] = ranch_server:get_listener_start_args(Ref), | ||
Protocol; | ||
info(Ref, protocol_options) -> | ||
get_protocol_options(Ref). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems completely unrelated and not necessary (I didn't see any change other than active/all_connections retrieval method having changed). Correct? So probably better to keep it all as it was and just replace the two relevant calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, now how did that end up in there... You're right, it is unrelated.
I had been playing with this quite a long time ago, then got distracted, then forgot. IIRC, my reason for this was so one could query a very specific info for a listener instead of all of it, which takes some extra time to collect only to be ignored,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to update the PR to master and remove this, I'll get to it on Wednesday myself.
src/ranch_conns_sup.erl
Outdated
{'DOWN', MonitorRef, process, SupPid, Reason} -> | ||
error(Reason) | ||
end. | ||
|
||
-spec start_protocol(pid(), inet:socket()) -> ok. | ||
start_protocol(SupPid, Socket) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not look necessary anymore, every calls are using the start_protocol/3 function.
src/ranch.erl
Outdated
ListenerSup = ranch_server:get_listener_sup(Ref), | ||
{_, ConnsSupSup, _, _} = lists:keyfind(ranch_conns_sup_sup, 1, | ||
supervisor:which_children(ListenerSup)), | ||
_ = [ConnsSup ! {remove_connection, Ref, self()} || {_, ConnsSup, _, _} <- supervisor:which_children(ConnsSupSup)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many lines are too long, for example this one. Please try to limit to around 100 columns. (Basically if it's not fully visible on the Github UI it's probably too long.)
You forgot to add a copyright header to the new file, can you write in a comment the copyright line for you and i'll add it in my copy. |
Ok, sure. Do you want your name in there or mine (both are ok with me), and 2011-2019 as the year or just 2019? |
|
Merged, thanks! |
one conns_sup for each acceptor