Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

The task was to build a simple parallel server in erlang that prints what it receives through a socket and alerts when the socket is closed.

-module(server).
-export([start/0]).


start() ->
    {ok, ListenSocket} = gen_tcp:listen(8211,[binary,{packet,4},{active, once}]),
    spawn(fun() -> accepter(ListenSocket) end),
  sleep().


sleep() ->
    receive
        cancel -> void
    end.

accepter(ListenSocket) -> 
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> accepter(ListenSocket) end),
    Proc = spawn(fun() -> worker(Socket) end),
    gen_tcp:controlling_process(Socket, Proc).



worker(Socket) -> 

    receive
        {tcp, Socket, Data} -> 
            io:format("Received ~p",[Data]),
            inet:setopts(Socket, [{active, once}]),
            worker(Socket);
        {tcp_closed, Socket} -> 
            io:format("Socket ~p closed",[Socket])
    end.

Is this the correct way of building such a thing?

share|improve this question
    
Is your incoming data formed into packets, preceded with four length-bytes? I guess no. There is more about {packet, PacketType} option – kitty Apr 10 '15 at 9:32

Let's look at your solution function by function. First, start/0:

start() ->
    {ok, ListenSocket} = gen_tcp:listen(8211,[binary,{packet,4},{active, once}]),
    spawn(fun() -> accepter(ListenSocket) end),
    sleep().

First, port 8211 is hard-coded. You might instead want to export both a start/0 and a start/1, the latter taking a port number argument, with start/0 calling start/1 with 8211 as the default. You don't need {packet,4} unless your Erlang client is also using that option, or your non-Erlang client precedes each packet with a 4-byte integer in network order that indicates the packet length. You probably also want to use the {reuseaddr,true} option so that you can stop the application and quickly start it again on the same port. With all these changes, we wind up with this:

start() ->
    start(8211).
start(Port) ->
    {ok, ListenSocket} = gen_tcp:listen(Port, [binary,{active,once},{reuseaddr,true}]),
    spawn(fun() -> acceptor(ListenSocket) end),
    sleep().

(Note that technically you don't need the sleep/0 call, but I'm sure you put it there to make the program easier to work with interactively from an Erlang shell, so I'm leaving it.)

Next, we have sleep/0:

sleep() ->
    receive
        cancel -> void
    end.

I would return ok here, since that's extremely common in Erlang. Using the atom void is not.

Next, we have acceptor/1:

acceptor(ListenSocket) -> 
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> acceptor(ListenSocket) end),
    Proc = spawn(fun() -> worker(Socket) end),
    gen_tcp:controlling_process(Socket, Proc).

The big problem here is the spawning of the worker process, and then handing control of the accepted socket over to it. There are two problems with this:

  1. The accepted socket inherits {active,once} from the listen socket. But what happens if a message arrives after the worker is spawned but before control of the socket is given to it? The message is queued to the controlling process, which is the acceptor process, not the new worker process. The worker process misses that message, and since the acceptor never checks it, the message just gets lost.
  2. Once the acceptor accepts a connection, it spawns a new acceptor. This is good. But it then spawns a worker, gives it the control of the socket, and then dies. This is pointless. Since the acceptor spawns a new acceptor, it can just continue and call worker/1 itself, and that way it can avoid the problem of changing the socket's controlling process, which means it won't miss any messages.

We can fix these problems by revising acceptor/1 like this:

acceptor(ListenSocket) -> 
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> acceptor(ListenSocket) end),
    worker(Socket).

And finally, we have the original worker/1 function:

worker(Socket) ->
    receive
        {tcp, Socket, Data} -> 
            io:format("Received ~p", [Data]),
            inet:setopts(Socket, [{active,once}]),
            worker(Socket);
        {tcp_closed, Socket} -> 
            io:format("Socket ~p closed", [Socket])
    end.

This function looks just right, no changes needed.

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.