ranch_server:count_connections/1 results in a negative number

[ninenines]

ranch_server:count_connections/1 results in a negative number

[fishcakez]

The problem occurs because cowboy_websocket (or any ranch_protocol) calls ranch_server:remove_connection/1[1], which leads to ranch_conns_sup reducing its current connection count[2]. However once the websocket (or any ranch_protocol that removes connections) exits the connection count is reduced again[3] and so the count is 1 less than the actual count for every connection that has removed itself and then exited.

In order to handle this properly the state of each connection needs to be kept. Currently the pids of connection are stored in the process dictionary with pid keys and true values. The value could be the state (e.g. current or removed) instead of true. This can be set if the Pid is added to the remove_connection message (e.g. {remove_connection, Ref, self()}):

case put(Pid, removed) of
    current ->
        loop(State, CurConns - 1, NbChildren, Sleepers);
    % Already removed, buggy ranch_protocol. Currently decrement regardless!
    removed ->
        loop(State, CurConns, NbChildren, Sleepers);
    % Bad message?! Currently decrement regardless!
    undefined ->
        erase(Pid),
        loop(State, CurConns, NbChildren, Sleepers)
end

On a child's exit the Pid is erased, which allows retrieval of its state:

case erase(Pid) of
    current ->
        report_error(Ref, Protocol, Pid, Reason),
        loop(State, CurConns - 1, NbChildren -1, Sleepers);
    % Already reduced the connection count for this child. Currently decrement regardless!
    removed ->
        report_error(Ref, Protocol, Pid, Reason),
        loop(State, CurConns, NbChildren - 1, Sleepers);
    % The exit signal is not from a child. Currently decremented regardless!
    undefined ->
        loop(State, CurConns, NbChildren, Sleepers)
end

This would also allow better behaviour when max_connections has been reduced. Currently acceptors are slept once they accept a socket if max connections has been exceeded. However as soon as a child exits an acceptor is woken up without considering the current connection count and max connection limit. Therefore in the case where the max connection limit is reduced below the current connection count, the limit is not being obeyed. Instead the state of the exiting process can be used to only wake up acceptors when appropriate:

case erase(Pid) of
    % Currently no check done for CurConns =< MaxConns when awakening an acceptor.
    current when Sleepers =/= [] andalso CurConns =< MaxConns ->
        report_error(Ref, Protocol, Pid, Reason),
        [To|Sleepers2] = Sleepers,
        To ! self(),
        loop(State, CurConns - 1, NbChildren - 1, Sleepers2);
    current ->
        report_error(Ref, Protocol, Pid, Reason),
        loop(State, CurConns - 1, NbChildren - 1, Sleepers);
    removed ->
        report_error(Ref, Protocol, Pid, Reason),
        loop(State, CurConns, NbChildren - 1, Sleepers);
    undefined ->
        loop(State, CurConns, NbChildren, Sleepers)
end

Also note that the awaking of acceptors should be added to the remove_connection clause. Currently this is not done and is a related bug:

case put(Pid, removed) of
    current when Sleepers =/= [] andalso CurConns =< MaxConns ->
        [To|Sleepers2] = Sleepers,
        To ! self(),
        loop(State, CurConns - 1, NbChildren, Sleepers2);
    current ->
        loop(State, CurConns - 1, NbChildren, Sleepers);
    removed ->
        loop(State, CurConns, NbChildren, Sleepers);
    undefined ->
        erase(Pid),
        loop(State, CurConns, NbChildren, Sleepers)
end

[1] https://github.com/ninenines/cowboy/blob/3d9078018d7f0a83a359b70c698d35e35fbb94f9/src/cowboy_websocket.erl#L62
[2]

ranch/src/ranch_conns_sup.erl

Line 143 in a410985

loop(State, CurConns - 1, NbChildren, Sleepers);

[3]

ranch/src/ranch_conns_sup.erl

Line 162 in a410985

loop(State, CurConns - 1, NbChildren - 1, Sleepers);

and

ranch/src/ranch_conns_sup.erl

Line 169 in a410985

loop(State, CurConns - 1, NbChildren - 1, Sleepers2);

[essen]

I agree with everything you say here. Sounds like you're pretty far already so I'm guessing there'll be a PR coming. :-)

"removed" makes sense but I'm not sure what "current" means, what's the opposite of "removed"?

[fishcakez]

I'm not sure why I used current, I think because of the CurCons (current connections) variable. The opposite of removed is added. I think included is better because the pid is included in the connection count.

Sure a PR can appear once I have time to add some tests.

[essen]

"included" sounds good. Thanks in advance!

[essen]

I am cherry-picking fishcakez@c3a3862 and adding a test. Test was very quick to write, and demonstrates that the issue is fixed. I also like active more than current or included.

Closing, thanks!