Add panic handler

[HigherOrderLogic]

See https://github.com/serenity-rs/poise/projects/1#card-70592577 for more info.

[HigherOrderLogic]

@kangalioo this has been tested and is ready for review.

[GnomedDev]

I don't believe poise should be catching these panics, as panics are catastrophic errors caused by programmer error, generally meaning state is no longer valid.

I can't even see this helping rust beginners, as it would teach them that panics are like exceptions in other languages and should be caught.

The only valid use of catch_unwind machinery in my head is FFI where it is "catch or UB", which this definitely isn't.

[tazz4843]

I agree with Gnome here, and to add to that, it's nowhere near guaranteed that panics will be caught. This definitely will trick some beginners I think.

[cheesycod]

I agree too, this sounds like a antifeature to me

[kangalio]

What about the use case of emitting an error message to the user on bot panic? I don't think the current situation is user-friendly, where the bot just doesn't respond whatsoever when it hits an internal error / panic @GnomedDev @tazz4843 @cheesycod

[HigherOrderLogic]

What about the use case of emitting an error message to the user on bot panic? I don't think the current situation is user-friendly, where the bot just doesn't respond whatsoever when it hits an internal error / panic

Does this mean that we would introduce poise's own Event enum?

[kangalio]

Does this mean that we would introduce poise's own Event enum?

What do you mean? I was responding to the other people, by the way. Also, thank you for making this pull request, I personally think it's an awesome future to have!

[HigherOrderLogic]

What do you mean?

I mean if we want to emit a "panic event" then we have to introduce our own event system, don't we?

[kangalio]

I mean if we want to emit a "panic event" then we have to introduce our own event system, don't we?

Who said we want to emit a panic event 👀 With "emitting an error message" I just meant something like ctx.say("Bot unexpectedly panicked")

[HigherOrderLogic]

With "emitting an error message" I just meant something like ctx.say("Bot unexpectedly panicked")

Oh, I see...

[GnomedDev]

Emitting an error to the user on panic feels like trying to emit an error to the user when the network dies. If it is possible (as in, panicking will actually unwind, instead of aborting), the only thing that could be emitted is "something went wrong" which a user would be able to see anyway because the bot didn't respond to them.

[kangalio]

That's an extreme comparison. The emitted error can be more specific than you suggest: "Sorry, the command crashed unexpectedly (). The crash has been recorded and will be investigated" or whatever. That's certainly better than dead silence

Also, a panic handler allows you to record and track panics in the first place, which is also good to fix bugs

[tazz4843]

This still feels like it will very much encourage people to catch panics and treat them as recoverable errors.

[kangalio]

I think we shouldn't forgo useful features just because they can be misused. Also, I think it's worth exploring new design spaces. For example, I impulsively thought #51 was just an indicator for bad practice, but I implemented it anyways and it turned out very useful every now and then - I think the same can happen with a panic handler.

And in any case, if newbie Rust programmers indeed starting panicking everywhere due to this change, it can always be deprecated and removed

[kangalio]

Status: I like this feature very much. I still need to go through this PR carefully and check if every place that executes user code is guarded with a catch_unwind. I'm currently procrastinating this

[kangalio]

It would be best to update the existing FrameworkError variants Setup, EventHandler, Command, ArgumentParse, CommandCheckFailed, DynamicPrefix to also be able to contain a panic payload instead of an instance of the global error type E.

But I'm not sure how to do that without annoying churn for users. The cleanest API would be to replace the error: E fields with error: PanicOrError<E>, where PanicOrError is:

pub enum PanicOrError<E> {
    Panic(Box<dyn Any + Send + 'static>),
    Error(E),
}

But then, users will have to change all their code that interacts with the error to destructure the enum and handle the panic too, including downcasting to &str/String. I need opinions from everyone: is that ok? Are there ideas for a better solution?

[tazz4843]

Personally I feel like that adds another annoying edge case for error handling. I would like a feature flag to be able to opt out of that if it's going to end up with that enum.

[GnomedDev]

Panic payloads are inherently dangerous as dropping them while handling a panic can cause an abort, this is one of the reasons I disagree with poise interacting with panic machinery (intended for unsafe FFI to be sound) at all.

[cheesycod]

If we are adding panic handling, i think it would be better to ensure that a panic just calls the handler and then ALWAYS kills the bot afterwards as a panic could mean that there's a unrecoverable error

[cheesycod]

I personally use panic = abort personally so I'm not the best to comment on this PR

[kangalio]

Personally I feel like that adds another annoying edge case for error handling. I would like a feature flag to be able to opt out of that if it's going to end up with that enum.

@tazz4843 I agree it's annoying. A feature flag would be an option, true. But not a great option imo, because feature flags are also annoying

Panic payloads are inherently dangerous as dropping them while handling a panic can cause an abort

@GnomedDev This is nothing special to panic payloads; anything that can panic while handling another panic will cause an abort. And this is a fair point, we need to avoid this trap if a panic handler were to be implemented. (For example tokio::spawn-ing the panic handler should work).

If we are adding panic handling, i think it would be better to ensure that a panic just calls the handler and then ALWAYS kills the bot afterwards as a panic could mean that there's a unrecoverable error

@cheesycod I still don't understand this radical view :/ What kind of panic can happen in a Discord bot that's so unrecoverable that you can't even post a Discord error message after it happens? What state can it even supposedly corrupt?

The one panic I can remember from my Discord bots right now was a bug in a graphing lib that panicked when given an empty x axis. When my bot hit that in a command, it just didn't respond anything. With a panic handler, I'd just write ctx.say(format!("Unexpected crash inside the bot: {}", panic)).await, and users would see that and everyone is happy.

Let me try to articulate my motivation here:

Panics are not an evil to be avoided always - it is just an error mechanism for unexpected states within the application. A panic is appropriate when the caller of the function typically doesn't need to consider the possibility of an error.

I believe many of the -> Result<_, Error> return types in a typical Discord bot codebase actually fall under that category. "Yeah technically the network could go down and the Discord message would fail to send, let's just return Result and slap an ? on every call" - this practice pollutes the meaning of Result.

If such errors that are always just propagated away disappeared from function signatures, the remaining errors that actually need specialized inline handling get the visibility they deserve. The mindset would shift from "Result? Let's use ?" to "Result? Let's check what can go wrong in practice and recover, depending on context". In the end, this will make for more robust programs.

[GnomedDev]

The problem is that you are disagreeing with the entire rest of the Rust ecosystem, and this will be another step to understanding error handling for beginners.

Error handling is complicated enough without "oh and poise also catches and makes you handle panic payloads". This wouldn't clean up network error conditions either as most serenity results cannot be unwrapped as permission failures or other runtime issues would then be raised to a panic.

If this is merged I will be forced to start using panic=abort or maintain a fork with this reverted.

[kangalio]

We continued this discussion in the Discord and tldr I was completely flooded with disagreement and nobody except me seems to like making the panic handler custom.

This, coupled with the fact that API design for this feature is a bit hard, means I'll just surrender and close this PR. Sorry @HigherOrderLogic and thanks again

[kangalio]

• I spoke to members of the Rust community Discord (link) and they said this is a totally valid use case of catch_unwind
• I also did some benchmarking and catch_unwind seems to be zero-cost when no panic is thrown (@GnomedDev had performance concerns about wrapping all command code in catch_unwind)
• I got an idea for a clean API, and a clean code implementation of it
• I found out there's no hazard with double panics either

Soo I guess I will revive this.

This will be an optional and backwards-compatible change and I will note in the docs that panics are not meant as a general error mechanism - please don't despair.