Defensive programming

[Samuel Rivas]

Joe Armstrong (AL/EAB) wrote:
> > Today I've found this piece of code I wrote some months ago:
> > 
> > acceptor(tcp, Module, LSocket) ->
> >     case gen_tcp:accept(LSocket) of
> >         {ok, Socket} ->
> >             case Module:start() of   
> >                 {ok, Pid} ->
> >                     ok = gen_tcp:controlling_process(Socket, Pid),
> >                     gen_server:cast(Pid, {connected, Socket}), 
> >                     acceptor(tcp, Module, LSocket); 
> >                 {error, Error} ->
> >                     {stop, {Module, LSocket, Error}}
> >             end;
> > 	{error, Reason} -> 
> > 	    {stop, {Module, LSocket, Reason}}
> >     end;
> 
> 14 lines of complex code - with a doubly indented case clause
> 
> > 
> > is that too defensive ? should I write it this way
> > 
> > acceptor(tcp, Module, LSocket) ->
> >     {ok, Socket} = case gen_tcp:accept(LSocket),
> >     {ok, Pid} = Module:start()
> >     ok = gen_tcp:controlling_process(Socket, Pid),
> >     gen_server:cast(Pid, {connected, Socket}), 
> >     acceptor(tcp, Module, LSocket);
> > 
> 
> vs 6 lines of linear code with no conditional structure.
> 
> How can you ask the question - you KNOW the answer.
> 
> Six lines of linear code is far better that 14 lines of code with
> conditional structure.
>
> [ ... snip ...]
>
> I just say:
> 
>       "Let it crash"
> 
> [ ... snip ...]
> 
> Error handling in Erlang is based on the idea of "workers" and
> "observers"
> where both workers and observers are processes.
> 
>       +-----------+                 +-----------+
>       |   Worker  |------->---------|  Observer |
>       +-----------+   error signal  +-----------+
> 

  I have a doubt about that:

  In my view, the former piece of code is _almost_ fine. It's fine
because on absence on errors it does exactly what you want; no more, no
less. I said "almost" because on errors it does not behave exactly as
I'd like.

  Elaboration: the error the observer gets is not the actual error, but
a "badmatch". Thus, I usually wrap OTPish functions like that:


-module(foo)

bar(Anything, AnotherThing) ->
  case an_otp_lib:bar(Anything, AnotherThing) of
    {ok, Value} ->
      Value;
    {error, Reason} ->
      erlang:error(Reason)
  end.


  Now, my code would read almost as proposed (getting rid of the 
{ok,Result} matches) and the observer can try-catch errors if needed,
obtaining the actual exit reason. This way, functions always return a
valid term, failing otherwise.

  However, that comes with some problems:

  - I have to wrap a lot of things
  - I can not easily distinguish expected errors from bugs (badmathces,
    undefs, etc) any more, since they fall in the same catch category.
  - I feel going against the tide since supervisors and gen_servers are
    not particulary happy with this philosophy (they stubbornly catch 
    error signals on startups and return {error, Reason} tuples; among
    other nuisances).

  I can solve the second point changing erlang:error() with exit() thus
catching exit signals, but it leaves a dirty feeling (I don't want to
exit really, I want to signal an error). The other two are solved with
a fair amount of adapting code. For example, I build my gen_servers using
these utility functions:


-module(foo_lib).

server_call(Server, Request) ->
    case gen_server:call(Server, Request) of
	{error, Reason} ->
	    exit(Reason);
	Value ->
	    Value
    end.

secure_call(Request, From, State, Module, CallFunc) ->
    try Module:CallFunc(Request, From, State) of
	Result when is_tuple(Result), size(Result) == 3 ->
	    Result;
	_ ->
	    %% Pretty defensive against annoying bugs
	    erlang:error(bad_call_return)
    catch
	error:Reason ->
	    {reply, {error, Reason}, State};

    end.


So in the server module, handle_call is implemented like this:


handle_call(Request, From, State) ->
    foo_lib:secure_call(Request, From, State, ?MODULE, handle_call2).

handle_call2({bar, Args}, _From, State) ->
    {Reply, NewState} = handle_bar(Args),
    {reply, Reply, NewState};


I wonder whether this "make it crash (TM)" approach is somewhat overkill.


Regards
-- 
	Samuel