[erlang-questions] erlang:raise/3 considered harmful?

[Garrett Smith]

On Sat, May 14, 2016 at 4:48 AM, Per Hedeland <per@REDACTED> wrote:
> Hi,
>
> I happened to want something like erlang:raise/3 for a library function
> - I didn't think it existed:-), but found that it does, although it
> comes with a semi-dire warning. The loop/3 function below does exactly
> what I want, specifically: if the fun provided by the caller of
> interate/3 crashes, the caller gets the same exception as if there had
> been no try ... catch, can handle it with hir own try/catch or not, but
> in any case receives full information about *what* went wrong in hir fun
> - all while the "protocol" spoken over the socket remains in sync.
>
> So, to take heed of the warning - is there a way to write an equivalent
> function *without* using raise/3 - or should it just be considered a
> case of "you really know what you are doing"? I believe I am of
> course:-), but I also think that the code below is a perfectly
> reasonable implementation of a side-effecty library function, and has no
> more to do with debugging or knowing what you're doing than any other
> code...

I think your case justifies the use of this scary function :)

That said, I don't think you actually want to use it. Your API need
improving I think.

The fact you're returning a tagged tuple {ok, State} puts this into
the "tagged tuple" function category. This in contrast to the "value
vs exception" category. See dict:find vs dict:fetch for a contrast of
the two patterns. Stick with one or the other - don't mingle them.

So I'd expect something like this:

try
  Fun(Data, State)
catch
  Class:Reason ->
    Err = {Class, Reason, erlang:get_stacktrace()}
    {error, {callback_error, Err}}
end

But onto other issues...

It's not clear what "Error" is from looking at the code - and I'd
rather not (as a reader) have to poke around type specs. If you expect
the form {error, Error} there, spell that out. E.g.

            case Res of
                ...
                {error, Error} ->
                    gen_tcp:send(Sock, "done"),
                    {error, Error}

If "Error" can be anything, you're on shaky ground API wise. You're
asking the caller of your function to handle {ok, Good}, arbitrary
exceptions, and arbitrary return values. I would be very cranky with
this interface.

Finally, the imperative style coding here really wants some functions,
in particular the calling and handling of the callback (but also
applies to the calling and handling of the socket read). I'd do
something like this:

handle_cb(apply_cb(Fun, Data, State), Socket, Fun)

apply_cb can then tuck away your logic for how to call this function.
I'd typically use "catch" (I don't care for try...catch), but this
would allow "throw" errors to pass through as results. You may or may
not want that - I would not want that certainly without a strong
defense. So this:

apply_cb(Fun, Data, State) ->
  try
    Fun(Data, State)
  catch
    Class:Reason ->
      {error, {cb_error, {Class, Reason, erlang:get_stacktract()}}}
  end.

Your handler then replaces your case expression:

handle_cb({continue, State}, Sock, Fun) ->
  gen_tcp:send(Sock, "continue"),
  loop(Sock, Fun, State);
handle_cb({done, State}, Sock, Fun) ->
  gen_tcp:send(Sock, "done"),
  {ok, State};
handle_cb({error, Err}, Sock, _Fun) ->
  gen_tcp:send(Sock, "done"),
  {error, Error};
handle_db(Other, Sock, _Fun) ->
  gen_tcp:send(Sock, "done"),
  error({unexpected_cb_result, Other}).

You get a lot of goodness here:

- Clearly separated concerns, each with its own name+args (i.e. function)
- Consistent use of tagged tuple pattern ({ok, Good}, {error, Bad})
- Explicit error information with enough detail to diagnose errors
- Intolerance and refusal to handle a misuse of this API (the error
for unexpected cb result)

And then just to see it in print:

loop(Sock, Fun, State) ->
  handle_data(recv_data(Sock), Sock, Fun, State).

You get the idea.

And I fully understand, this was not your original question :)