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

[Garrett Smith]

On Sun, May 15, 2016 at 8:35 AM, Per Hedeland <per@REDACTED> wrote:
> 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.

But that's not at all what's going on here. You're injecting behavior
(side effects) here in an otherwise unexpected crash. You're taking
action to send something to a socket. Then you're re-raising the
function.

If you look at the higher order functions in core, of course they let
callback exception propagate - but they don't sneak in behavior and
pretend they didn't.

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

I'm not suggesting anywhere to return an error tuple for a badarg.

I'm suggesting that you use a consistent interface, which returns a
tagged tuple with ok/error for handled scenarios. If you're altering
the normal course of a true exception (i.e. unanticipated and
unhandled) by taking some "cleanup" action, you ought to not pretend
that it's unexpected and unhandled. Communicate it as an explicit
error, providing all the details needed to detect and troubleshoot the
problem.

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

Yes, but again - when you perform an operation in the middle of this
propagation, you ought to let the caller know. You're sending bytes to
a socket _that your caller gave you_. That's not important news to
share?

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

You're not obscuring anything here. You're adding _more_ information.

You're also giving the caller a _consistent_ interface for detecting errors.

Other exceptions - the unhandled variety - they pass through. I would
not for example ever want to see this:

my_fun() ->
   try
      something()
   catch
     C:Err ->
        {error, {no_idea_really, {C, Err, erlang:get_stracktrace()}}}
   end.

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

Stunningly bad idea. You are actively discouraging your caller from
taking further action on an error _that you know about and acted on_.

What benefit do you think you're providing here?

You have two choices: return as an error or generate an exception. In
either case you ought to provide the original error info. In exception
form:

error({cb_error_but_chillax_i_sent_done, OriginalErrorInfo})

But I would not do this - you're already communicating errors with
tagged tuples. Now you're asking your user to handle _known errors_ in
two different ways.

Here's their code:

try iterate(Sock, Fun, State) of
  {ok, State} -> handle_good(State, Sock);
  {error, Err} -> handle_error(Err, Sock)
catch
  C:Err -> handle_error({C, Err}, Sock)
end

handle_error can of course sort out what's to be done based on the
error, but the mix-and-match of tuple and exception handling is simply
a bad interface.

BUT, the real fun is sorting out the case where "done" was sent due to
an unhandled callback exception!

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

Correct - if you let the exception propagate without explicit handling
and action, no issue whatsoever.

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

Experienced programmers (language aside) don't let bad state linger
for long. I wonder when an error in that case would be detected - and
how much useful context information would still be available.

> Or maybe it's actually a feature?:-)

Yeah, no, it's not.

> Seriously, I have a hard time losing
> sleep over the absence of checks for things that aren't actual problems

Yeah, no - a violated protocol that goes undetected is an actual problem.

> but in the *real* code, that I tried to strip down in order to focus on
> my point (obviously failing miserably:-)

Your point I believe was a question, which seemed to seek affirmation
for the use of a documented function in core.

You've gotten some pretty good content in response - or, the community
has. That's often the point of responses to questions here - to go on
record with what one thinks is generally helpful information, for the
sake of not just the OP but for everyone. It often spurs fruitful
discussion about any variety of topics.

I had to look up this 'raise' function because I never heard of it.
Why had I never heard of this function? Maybe I'm a rookie, or maybe
it's just rarely used. Why would you ever use this function? You've
highlighted one reason - to trick callers.

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

If the original code uses cogent functions, rather assigning case
expressions to variables in hard-to-read imperative style, in the
interest of avoiding TLDR in code, post that next time.

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

This is certainly not atypical Erlang - invalid callback results are
often highlighted with an explicit error (e.g. see gen_server). It's
hardly _bad_. It's arguably unnecessary - some may enjoy translating a
function/case clause reported in obscure stacks to a mistake they
made.

In any case, to your original point, which I think everyone is
following clearly enough, go for it!