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

[Per Hedeland]

Garrett Smith <g@REDACTED> wrote:
>
>I think your case justifies the use of this scary function :)

Thanks.:-) Though given the other things that you wrote, I have to
wonder *why* you think so...

>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.

(Hm, I guess I need to post more, to avoid being treated like a
rookie...:-) I won't go into that other than to say that yes, this
function is firmly in the "tagged tuple" category, for what I am
convinced is good reasons. But even such a function may have bugs, and
*I* would certainly not expect every such function to wrap each of its
clauses in try/catch in order to make sure it returns {error,
{i_have_a_bug, ClassAndReasonAndStacktrace}} instead of just letting the
exception propagate to the caller.

The question of what do with bad arguments passed to such a function is
perhaps not as crystal clear, but I think you will agree that the
"standard Erlang way" is that an exception is raised in that case too -
the function *may* do argument validation via guards, but not in order
to return a tagged tuple when there is a bad argument - the result will
in such cases typically be a function_clause exception. Just as a data
point related to my example, try gen_tcp:recv(foo, 0). If one of the
arguments happens to be a fun, this doesn't really change anything - if
the fun crashes when called, you can expect that the exception is just
propagated to the caller that provided the bad fun in the first place.

And there are good reasons for this of course - the {error, Foo} tuples
that this type of function may return are typically things that the
caller actually needs to be prepared for and handle in "normal"
operation - the peer closed its socket, you don't have permission to
open the file, etc etc - while the exceptions are not.

>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

... and thus I *really* don't want to write code like that, coercing an
exception into a "normal" error, that the caller *has* to handle - or
possibly, by not handling it, generate a "secondary" case_clause
exception or the like, which just obscures the real problem.

The whole point of my example was to show a function that had a need to
interfere with this "standard" pattern of letting exceptions propagate,
in order to do some "cleanup" of side effects. And this should be done
in a transparent manner, i.e. the end result for the caller should be
the same as if there was no interference. If it wasn't for this cleanup,
there would be no reason to do the try/catch in the first place. And I
don't think the function without try, catch, and erlang:raise/3 would
have elicited your comment about "mingle interfaces", even though its
"interface" had been exactly the same.:-)

>But onto other issues...

Well, this brings us way over the TL;DR threshold, and is also
completely unrelated to what I'm asking about, but OK...

>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.

Semi-good point:-), though the Error you are talking about here is what
is returned from the fun passed in by the caller. It's spec is that it
must return {continue, State} | {done, State} | {error, Reason} - if it
returns something else, it will also be returned to the caller - serves
him right for providing a fun that didn't comply with the spec.:-) Or
maybe it's actually a feature?:-) Seriously, I have a hard time losing
sleep over the absence of checks for things that aren't actual problems,
but in the *real* code, that I tried to strip down in order to focus on
my point (obviously failing miserably:-), the caller would actually be
rewarded with a function_clause exception in that case.

>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:

Sure, and that's actually pretty much what the *real* code already looks
like. Main differences is that it doesn't do this:

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

- which I consider Bad(tm) for the reasons given above, nor this

>handle_db(Other, Sock, _Fun) ->
>  gen_tcp:send(Sock, "done"),
>  error({unexpected_cb_result, Other}).

- which I consider at least "atypical Erlang" and "unnecessary" and even
a little "bad".

--Per