Storing supervisor children PIDs

Question

I'm Erlang beginner and trying to implement my first Erlang application. It is a network monitor tool which should execute ping request to specified host. Actually sending ICMP is not an aim, I'm more interested in application structure. Currently I have monitor_app, monitor_sup (root sup), pinger_sup and pinger (worker). This is pinger_sup:

-module(pinger_sup).
-behaviour(supervisor).
-export([start_link/0, start_child/1, stop_child/1]).
-export([init/1]).

start_link() ->
    supervisor:start_link({local, ?MODULE}, ?MODULE, []).

start_child(Ip) ->
    {ok, Pid} = supervisor:start_child(?MODULE, Ip),
    put(Pid, Ip),
    {ok, Pid}.

stop_child(Ip) ->
    Children = get_keys(Ip),
    lists:foreach(fun(Pid) ->
            pinger:stop(Pid)
        end,
        Children).

init(_Args) ->
    Pinger = {pinger, {pinger, start_link, []},
        transient, 2000, worker, [pinger]},
    Children = [Pinger],
    RestartStrategy = {simple_one_for_one, 4, 3600},
    {ok, {RestartStrategy, Children}}.

And pinger itself:

-module(pinger).
-behaviour(gen_server).
-export([start_link/1, stop/1, stop_ping/1, ping/1, ping/2]).
-export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2, code_change/3]).
-define(PING_INTERVAL, 5000).

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

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

stop_ping(Ip) ->
    pinger_sup:stop_child(Ip).

ping(Ip) ->
    pinger_sup:start_child(Ip).

ping(_Ip, 0) ->
    ok;
ping(Ip, NProc) ->
    ping(Ip),
    ping(Ip, NProc - 1).

init(Ip) ->
    erlang:send_after(1000, self(), do_ping),
    {ok, Ip}.

handle_call(_Request, _From, State) ->
    {noreply, State}.

handle_cast(stop, State) ->
    io:format("~p is stopping~n", [State]),
    {stop, normal, State}.

handle_info(do_ping, State) ->
    io:format("pinging ~p~n", [State]),
    erlang:send_after(?PING_INTERVAL, self(), do_ping),
    {noreply, State};
handle_info(Info, State) ->
    io:format("Unknown message: ~p~n", [Info]),
    {noreply, State}.

terminate(_Reason, State) ->
    io:format("~p was terminated~n", [State]),
    ok.

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

It lacks of comments but I think it's very straightforward and familiar to any Erlang developer. I have few questions about this code:

1) Is *erlang:send_after* in init a good solution? What is the best practise if I need process start doing the job immidiately it was spawned without triggering his functions from outside?

2) Currently I initiate ping using pinger:ping("127.0.0.1"). command. pinger module ask pinger_sup to start child process. After the process was started I want to stop it. What is the preferable way of doing it? Should I terminate it by sup or should I send him stop command? How should I store the process PID? Currently I use process dictionary but after implementing it I've realized that the dictionary actually doesn't belong to sup (it's a shell dictionary or any other stop_child caller). In case I use ETS and the sup was terminated would these dead process PIDs stay in ETS forever causing kind of memory leak?

I appreciate any answers and code comments.

Thanks!

Answer 1

1) Is erlang:send_after in init a good solution?

No.

What is the best practise if I need process start doing the job immidiately it was spawned without triggering his functions from outside?

See here. Put timeout of 0, and then do:

handle_info(timeout, State) ->
    whatever_you_wish_to_do_as soon_as_server_starts,
    {noreply, State}.

Zero timeout means that server will send itself a "timeout" info as soon as it finished init, before handling any other call/cast.

Also, see timer module. Instead of recrsively calling send_after in handle_info(do_ping, State), just start the timer and tell him to send you "do_ping" every ?PING_INTERVAL.

2) Currently I initiate ping using pinger:ping("127.0.0.1"). command. pinger module ask pinger_sup to start child process. After the process was started I want to stop it. What is the preferable way of doing it? Should I terminate it by sup or should I send him stop command?

You should send him a stop command. Why killing the gen_server using a supervisor, if gen_server can do it by itself? :-)

How should I store the process PID? Currently I use process dictionary but after implementing it I've realized that the dictionary actually doesn't belong to sup (it's a shell dictionary or any other stop_child caller). In case I use ETS and the sup was terminated would these dead process PIDs stay in ETS forever causing kind of memory leak?

ETS tables are destructed as soon as owner process terminates. So, no memory leak there.

But, storing PIDs the way you do it is not "the erlang way". Instead, I propose making a supervisor tree. Instead of putting all pinger workers under the pinger_sup and then remember which worker pings which IP, I propose that pinger_sup starts a supervisor which handles the given IP. And then this supervisor starts needed number of workers.

Now, when you wish to stop pinging some IP, you just kill the supervisor for that IP, and he'll automagically kill his children.

And how would you know which supervisor deals with which IP? Well, put the IP in the supervisor's name :-) When spawning the supervisor which deals with an IP, do something like this:

-module(pinger_ip_sup).

start_link(Ip) ->
    supervisor:start_link({global, {?MODULE, Ip}}, ?MODULE, []).

Then, when you wish to stop pinging an Ip, you just kill the supervisor by the name {global, {pinger_ip_sup, Ip}}, and he'll kill his children :-)

edit concerning comments:

If you wish to handle error which produce timeouts, you can reserve a state variable which will tell you if it is timeout produced by init or timeout produced by error. For instance:

-record(pinger_state, {ip, initialized}).

init(Ip) ->
    State = #pinger_state{ip = Ip, initialized = false}.
    {ok, State, 0}.

handle_info(timeout, State#pinger_state{initialized = false}) ->
    whatever_you_wish_to_do_as soon_as_server_starts,
    New_state = State#pinger_state{initialized = true}.
    {noreply, New_state}.

handle_info(timeout, State#pinger_state{initialized = true}) ->
    error_handling,
    {noreply, State}.

That way you can use this mechanism and handle timeout errors. But, the real question here is: do you expect timeout errors at all?

As for timer: yes, timer has some overhead in case you plan on doing DDOS attacks or something like that :-D If you do not plan on creating and canceling timers like crazy, it is far more elegant to use timer module. That is a design choice you have to make: do you want a clean and elegant code, or code which can stand when everything other breaks. I doubt you need the latter. But, you know best.