Add documentation on returning via throw #5450

[bucko909]

This is a proposed documentation amendment to cover the undocumented behaviour whereby gen_server:try_dispatch will catch throw and treat the reason as the function's return value (used in global_group for example).

[github-actions]

CT Test Results

    2 files     95 suites   35m 23s ⏱️
2 142 tests 2 093 ✅ 48 💤 1 ❌
2 451 runs  2 400 ✅ 50 💤 1 ❌

For more details on these failures, see this check.

Results for commit 077f9ed.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[garazdawi]

Hello!

There are no tests that this actually works for gen_server, so if we want to extend the documentation to guarantee that this should work then there need to be new tests written. Since gen_statem allows this type of return, I think that we should add it to gen_server as well.

global_group (nor any other module that I'm aware of in Erlang/OTP) does not use this undocumented feature. It wraps all calls in a try catch that catches the throw and returns the correct value.

[bucko909]

@garazdawi you make some good points (I didn't actually check the modules I mentioned; I only searched for throw({noreply,). I propose that at least one of the following things should happen:

• Change this patch to "letting exceptions of class throw bubble up to gen_server will result in undefined, undocumented behaviour, which may change in future versions."
• If this feature is emulated in all standard modules, perhaps the better solution is simply to deprecate it and remove it from the behaviour modules. The only reason I'm submitting this patch is that the end of a 2 hour investigation into a system bug (getting a bad return value from a callback handler) discovered an uncaught throw which hit this clause: I'd prefer it gone, personally. If people need it, it's easy enough to add back in.
• Some tests should be written in gen_server (and anything else that does the same thing) to verify the behaviour.

I don't know which of these is the way forwards, and the first might be a sensible stopgap for the next release if it's controversial. What do you think?

[mikpe]

I wonder if that comes from ancient catch usage. I've seen throws being mis-labelled as normal returns elsewhere for that reason.

[max-au]

IIRC that's been discussed on the Erlang Forums (with no specific conclusion). I'm not sure if this behaviour is in fact desired - so far I know no other generic behaviour (gen_statem?) that does it.
If this is desired design, then probably it should be coming with a set of other guidelines for using different exception classes (similar to what we have internally, when throw is not supposed to cross module boundary, and exit means that the current process state is no longer safe to use as it can have garbage in the process dictionary or unexpected messages in the message queue, - like it was before with gen_server late replies, before aliases were the thing).

[RaimoNiskanen]

@max-au: gen_server, (the obsoleted gen_fsm), gen_event and gen_statem all implement this "feature" of equating a thrown value with a regular return. I suspect, as @mikpe, that this stems from using catch Mod:Callback() before try...catch existed.

When I wrote gen_statem I followed the pattern of the other generic behaviours, and saw reasonable use cases such as throwing {keep_state_and_data,[postpone]} from wherever you like, so I documented it in gen_statem.

[michalmuskala]

The throw-based returns have the added downside of making building abstractions on top of gen_server rather complicated - you need to guard against custom callbacks potentially throwing and handling those cases every time - otherwise it's easy to either break the abstraction (by throw-returning inner state, rather than the abstracted one), or by getting very confusing error messages on erroneous uncaught throws.

In practice few libraries handle this correctly.

[bucko909]

If the behaviour were to be preserved, I think the correct way to do it would be via a tagged exit reason, much like how gen_server:cast messages are sent as {'$gen_cast', Message}. The behaviour could then implement something like gen_server:throw_early_return(Value) to generate the correct shape of exit. This has the additional advantage that if something does accidentally catch the exit, the source of the exit will be clearer, because it'll be something like throw:{'$gen_early_return', Value}. It also uses the primary mechanism of alerting a developer to the existence of a feature (the existence of a function or callback), making it more discoverable.

I prefer the approach of forcing implementations to write their own exception handling, though, and I think we actually use this in our code-base, not having known about the throw bug/feature until finding it recently.

[RaimoNiskanen]

I think some tests should be added, that we simply keep this ancient behaviour behaviour, and document it.

[bucko909]

OK, so I was looking at what tests would be needed. For gen_statem the bug/feature seems to be tested by having a few of the callbacks in gen_statem_SUITE return via throw, though there is no test which explicitly does so. Would this be an acceptable approach for gen_event, gen_fsm, and gen_server?

(Searching implementations to verify these modules indeed have this bug/feature was somewhat confusing as it looks like, as @mikpe suggests, it's basically down to use of catch Mod:handle_call(...) which doesn't distinguish between returning and throwing, though I wasn't helped by some try ... catch handlers which don't explicitly write throw: in their clauses. I guess this is the original cause of the feature.)

[RaimoNiskanen]

A few callbacks that uses throw/1 in the test suites for gen_server, gen_event (and gen_fsm) would be sufficient, just to exercise the code path and to etch the feature into the test suites.

Handlers that do not use try ... catch throw:Result but instead try ... catch Result are not the origin of this feature; it has to be the use of catch Mod:Callback(...) from before try ... catch ... end existed. throw is the default exception class in try ... catch Class:Reason ... end and I have used that to mark when the code only handles a non-local return (throw/1) as opposed to exceptions of different classes. At least that has been my intention.

[bucko909]

Mmm, apologies, I conflated two things there -- the "default throw" shape made just grepping for "throw" inaccurate, but clearly isn't the original source. The catch F() shape was likely both the original source and made grepping inaccurate. My only point on the "default throw" thing is that things like this that "feel" nice can make macro understanding of the code harder. I suspect this, too, dates back to a time when the only catchable class was throw.

[RaimoNiskanen]

@bucko909: From what I remember, catch Expression has always caught all 3 classes of exceptions:

catch throw(4711). % -> 4711.
catch exit(4711).  % -> {'EXIT', 4711}.
catch error(4711). % -> {'EXIT', {4711, Stacktrace}}

So throw(X) passes X verbatim, and the others wrap the Reason in an {'EXIT', _} tuple. This conflates non-local returns, throw(X), with error handling.

When using this, unwrapping the EXIT tuple, to detect error you also get exit in the same bin and cannot distinguish a throw from a normal return value. It is also impossible to know if throw(X) or value return has been used to fake an EXIT tuple

When then try...catch...after...end construct was introduced, the fact that one of its uses was, like the old catch Expression, non-local returns, merited that the default class should be throw. Then you can write try Expression catch Thrown -> Code end if you want to handle only non-local returns. This also indicated that non-local returns are conceptually different from exceptions.

[bucko909]

I've pushed proposed tests for gen_server -- is this approach the right one? If so, I can do similar in the other two modules (and document them) and we can put this to bed.

Have rebased as the branch is oooooolde.

[bucko909]

@garazdawi ?

[bucko909]

I've rebased this change onto master, removing the documentation changes -- it seems like the documentation already included a phrase equivalent to this after it was refactored to use -moduledoc.

I still await advice on if the testing is sufficient, and hence should be added to the other two modules.

[bucko909]

(Looks like I may have caused test failures with the rebase? I'll have to check those out, I guess -- but would still appreciate feedback on style beforehand.)

[RaimoNiskanen]

I think the test code looks good enough.
But it didn't compile, 2 tests were failing. I have made 3 fixup! commits for that.

The first GitHub action log mentions our HowTos for DEVELOPMENT and TESTING. Please check them out for how to run in this case only stdlib gen_server_SUITE locally, to debug the PR test code...