fix: properly normalize erlang errors before emitting telemetry and logging crash_reason

[grzuy]

Hi there 👋

Minor follow up for a053c1c.

Analogous code in plug_cowboy: https://github.com/elixir-plug/plug_cowboy/blob/ced253f035f2a07ebed17cfacb64d4f16357a750/lib/plug/cowboy/handler.ex#L51-L55.

Thanks!

[mtrudel]

What's the reason for this change? Is there an issue that comes up with MatchError that doesn't with a simple raise?

[grzuy]

Is there an issue that comes up with MatchError that doesn't with a simple raise?

Yes, sorry for the lack of explanation.

Elixir's raise directly errors with an elixir's RuntimeError struct.

However, :one = :two or 1 / 0 will exit with {:badmatch, :two} and :badarith respectively, to name a few examples from https://www.erlang.org/doc/system/errors.html#exit-reasons.

Those need to be normalized with Exception.normalize somewhere, to become well-known elixir Exception structs (https://github.com/elixir-lang/elixir/blob/8deaaf4117fef24f516c12dc40989b9772a6d763/lib/elixir/lib/exception.ex#L2357-L2494).

The symptom in bandit right now is only that the non-normalized exit appears in crash_reason value.

The reason why is properly appearing normalized in the logged text message is because the existent Exception.format before Logger.error in

bandit/lib/bandit/pipeline.ex

Line 238 in 177b053

Logger.error(Exception.format(kind, reason, stacktrace), logger_metadata)

, which does the normalization behind the scenes: https://github.com/elixir-lang/elixir/blob/8deaaf4117fef24f516c12dc40989b9772a6d763/lib/elixir/lib/exception.ex#L134-L137.

[mtrudel]

I see. I wonder if we should be doing that for all errors (ie continue to have the Plug.Conn.WrapperError pattern match be 'dumb' and just pass the value through unchanged) and normalize in the other handle_error clauses. Does that make sense, or is it only needed in the narrow case of wrapped errors?

[grzuy]

Yes, I think you're right.
That's regardless of whether it's wrapped or not.

[grzuy]

Updated.

[grzuy]

Also now realized this will fix/normalize the emitted telemetry exception.

Updated PR title.

[mtrudel]

Looks great! Thanks for the continued work!

[mtrudel]

@grzuy I just noticed that this PR introduced console warnings during tests; would it be sufficient for us to explicitly raise a MatchError / ArithmeticError in the relevant places?

[grzuy]

@grzuy I just noticed that this PR introduced console warnings during tests;

Hey @mtrudel, sorry about that.

#459 should fix one.
Will also fix the other warning soon-ish.

would it be sufficient for us to explicitly raise a MatchError / ArithmeticError in the relevant places?

No, because the test cases are there to test that when original erlang :badarith or {:badmatch, x} are raised, Bandit properly normalizes. If we directly raise the Elixir normalized exception we wouldn't be testing that.

[grzuy]

@mtrudel I think this should fix it #459 🙏