current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I have two tasks:

Write a function that starts and monitors several worker processes. If any of the worker processes dies abnormally, restart it

Here is the code that I built:

proc() -> 
    receive 
        p -> exit("I need to die")
    end.


start_and_monitor(0) -> 
    spawn_monitor(proc()),
    receive 
        {'DOWN',_,process,_,_} -> spawn_monitor(proc())
    end;
start_and_monitor(Int) -> 

    spawn_monitor(proc()),
    start_and_monitor(Int - 1).

Write a function that starts and monitors several worker processes. If any of the worker processes dies abnormally, kill all the worker processes and restart them all.

Here I thought that instead of monitoring all of the process I could link all the small processes and monitor a major process that is linked to all of them. Here is the code:

start_and_monitor_but_kill(Int) ->
{Pid,Ref} = spawn_monitor(start_monitor_kill(Int)),
receive
    {'DOWN', Ref, process, _, _} -> start_and_monitor_but_kill(Int)
end.


start_monitor_kill(0) ->
    spawn_link(proc());
start_monitor_kill(Int) ->
    spawn_link(proc()),
    start_monitor_kill(Int-1).

Is there any way that I can improve my code? Am I using all the erlang primitives correctly ? Any criticism is highly appreciated.

Also I am very curious if I used the primitives correctly. For example I used the monitor process without giving a ref. Is this acceptable in this case?

share|improve this question
    
This code is not valid. see the definition of spawn_link/1, and spawn_monitor/1 at erlang doc –  Pascal Mar 10 at 12:12

1 Answer 1

up vote 2 down vote accepted

In your code you're calling spawn_monitor/1 like this:

spawn_monitor(proc()),

where proc/0 is a local function, but this doesn't do what you think it does. First it calls proc/0, which blocks in a receive until it gets a message. Normally it would return the value of receive, but your receive calls exit/1 so there's no return. So, what happens is that the first call into proc/0 blocks until it receives a message, at which point it exits, and since it was running not in a spawned process but in the calling process — the process where the caller is running — the caller exits too. Nothing ever gets spawned or monitored. (This same problem exists in your second code example too.)

What spawn_monitor/1 wants is a function, so you should be calling it like this:

spawn_monitor(fun proc/0),

If you make that change everywhere, then the code works, but there's one other thing: your start_and_monitor(0) function clause should not be calling spawn_monitor before calling receive, otherwise Int+1 rather than Int processes will be spawned and monitored.

As for the second code example, monitors and processes are cheap, so there's no point in trying to bundle them together into a single monitoring point.

And finally, you might consider solving this problem not with monitors but rather with links. A monitor allows a process to watch an unrelated process, but if one process is spawning another, then they're related, so links are a viable approach.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.