[erlang-questions] tcp connections dropped in gen_server

[Ladislav Lenart]

Hello.

[I hope it's ok that I reply to the list - someone else might find this
information useful as well. I am mentioning this only because you keep
replying to me privately.]

On 6.9.2011 17:52, Reynaldo Baquerizo wrote:
>>>> Also, if I understand the code correctly, the newly created connection
>>>> processes (acceptors) are not supervised. To prevent future problems
>>>> with this I strongly recommend you to modify your code slightly as
>>>> suggested in the book "Erlang and OTP in action".
>>>
>>> I didn't feel the need to supervised those connections. I fail to see
>>> the difference between leaving them unattended and simple_one_for_one
>>> with no restart.
>>
>> There was a comment tcp_sup:init/1 saying "tune for production demands" to
>
> Achh.. I read it as "tuned" :-)

Good one! I have to be careful next time I invent "helpful comments" :-)


>> give you a hint to change the restart strategy according to your needs. The
>> advantage of having the connection processes supervised (with suitable
>> restart
>
> Will those processes keep their state when restarted?
> For instance, the socket will be closed or the restart will happen earlier?

They won't be restarted. The socket will be closed because its controlling
process (the one that performed gen_tcp:accept/1) just died (and the socket
was linked to it). My sole aim was to put connection processes under
supervision. This way you will be informed about their abnormal crashes.
I proposed the following supervisor (in tcp_sup):

init([ListenSocket]) ->
     Server = {tcp_srv, {tcp_srv, start_link, [self(), ListenSocket]},
               temporary, brutal_kill, worker, [tcp_srv]},
     RestartStrategy = {simple_one_for_one, 0, 1},    % <-- tune for production demands
     {ok, {RestartStrategy, [Server]}}.

As you can see from the child specification (Server):
  * temporary - Instructs the supervisor to never restart its terminated children.
  * brutal_kill - Instructs the supervisor to kill its children (via exit(ChildPid,
    kill)) when it itself is about to terminate without giving them any chance to
    react whatsoever (e.g. clean up) but also without any possibility of failure
    of this operation. The brutal_kill is the only way to terminate the one process
    blocked indefinitely in call to gen_tcp:accept(ListenSocket).

It makes very little sense to restart these processes (to me), because
the TCP connection will die as well. The external client can reconnect
and start anew. Or am I missing something?


> I use a gen_server to encapsulate some state, does it matter? your
> example of tcp_srv isn't a gen_server.

Well, neither was yours, I just kept it that way to minimize changes
in your original code :-)

But gen_server it is...

NOTES:
  * Again, I haven't even made an attempt to compile the following code.
  * The tricky bit is to postpone the initialization (i.e. gen_tcp:accept/1).
  * The server has no application specific state. However it should be
    straightforward to add it, preferably with something like
        -record(state, {supervisor, listen_socket, socket, ...}).


%%% TCP connection process
-module(tcp_srv).

-behaviour(gen_server).

-export([start_link/2, stop/1]).
-export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2, code_change/3]).


%%% API

start_link(SupPid, ListenSocket) ->
     gen_server:start_link(?MODULE, [SupPid, ListenSocket], []).

stop(Pid) ->
     gen_server:cast(Pid, stop).


%%% Callbacks

init([SupPid, ListenSocket]) ->
     %% Return from the init now because the supervisor is waiting for me.
     %% Postpone my initialization for later.
     %% accept will be the first message I will receive and I will block
     %% there indefinitely.
     self() ! accept,
     {ok, {SupPid, ListenSocket}}.

handle_call(Msg, _From, State) ->
     {reply, {error, {unknown_request, Msg}}, State}.

handle_cast(stop, State) ->
     {stop, normal, State};
handle_cast(_Msg, State) ->
     {noreply, State}.

handle_info(accept, {SupPid, ListenSocket}) ->
     {ok, Socket} = gen_tcp:accept(ListenSocket),
     tcp_sup:start_child(SupPid, ListenSocket),    % <-- Instruct the tcp_sup SupPid to start new acceptor process.
     error_logger:info_msg("New connection from ~p~n", [Socket]),
     inet:setopts(Socket, [binary, {nodelay, true}, {active, true}]),
     {noreply, Socket};
handle_info({tcp, Socket, Data}, State) ->
     error_logger:info_msg("Messaged received from ~p: ~p~n", [Socket, Data]),
     %% comm_lib:handle_message/3 is expected to return NewState.
     {noreply, comm_lib:handle_message(Socket, Data, State)};
handle_info({tcp_closed, Socket}, State) ->
     error_logger:info_msg("Device at ~p disconnected~n", [Socket]),
     {stop, normal, State};
handle_info(_Any, State) ->
     {noreply, State}.

terminate(_Reason, _State) ->
     ok.

code_change(_OldVsn, State, _Extra) ->
     {ok, State}.

> I think I will reestructure my supervision tree as follow. One root
> supervisor with one_for_one strategy, a child supervisor
> (simple_one_for_one) for tcp client connections and child gen_server
> worker, pretty much like the example hanging out there.

Sounds good to me.


Ladislav Lenart