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

[Garrett Smith]

I've made a mistake here and I need to apologize for it. Per, I'm
simply in no position to take such a tone with you. You indeed are no
rookie, and I over stepped my bounds here in a terribly embarrassing
way.

I'll turn my attention to cleaning up my own code and save the
lectures for myself!

On Sun, May 15, 2016 at 11:00 AM, Garrett Smith <g@REDACTED> wrote:
> 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!