The default session_error_handler for Bunny::Session is a major footgun

[david-krentzlin]

Bunny::Session introduced the concept of a session error handler. This error handler is used to signal errors, that happen in different subsystem of a bunny session. Most notably in the threads created by ReaderLoop and Bunny::Transport.

The default error handler is Thread.current, which is the thread that created the bunny session.
The issue occurs because the session error handler is used in a different thread in the ReaderLoop.
Also applications might use the channel-per-thread pattern so any error in the transport due to channel operation in one of those threads will raise using the error handler.

This means that any error in those threads will raise an async exception in the thread that created the session.

This means that control flow of that thread can be effectively disrupted at every safe-point during execution.
This is not only hard to defend against, because you would have to guard effectively every line of code, but also difficult to debug,
because it's not obvious that exceptions can occur everywhere unless you happen to know about Bunny::Session.
Execution can be interrupted even in code like ensure or between retries, which can leave apps in a very bad state, with execution paths
that seem impossible if you look at the lexical information in the code.

In short, I think the default session error handler should be changed.
One possible implementation, would be an error handler that records if an error has occured, and allows to surface that information at a point where the thread that holds the bunny session is ready to handle it. Either by adding a method to raise any pending errors, or by automatically raising the error the moment, code tries to use a component that doesn't work anymore.

WDYT?

[michaelklishin]

:session_error_handler was introduced in #597, which assumed that the user will provide an implementation that responds to :raise. Which, in turn, can accumulate exceptions.

For 3.0.0, we can change the default to such an "exception accumulator". You are welcome to submit a PR.

I am 100% certain that someone will complain that this change would make something hard to debug but since the handler is only used when automatic connection recovery is disabled, such users supposedly known what they are doing and can provide an implementation that fits their needs.

[david-krentzlin]

Yes, that makes sense. I am pretty sure someone depends on the current way of doing things.

I need to double check but is it really true that this is only used when auto-recovery is disabled? The transport will use it, when someone tries to publish while the session is trying to recover no?

I have an implementation for an error handler that does what I proposed. I will try to find some time to make it suitable for general use.

[michaelklishin]

@david-krentzlin any updates on the aforementioned error handler implementation?