Safely handle channel creation errors on initiating connections and top-level supervisors#16598
Safely handle channel creation errors on initiating connections and top-level supervisors#16598Ayanda-D wants to merge 4 commits into
Conversation
in both network and direct connection types, to allow graceful termination of connections when such errors occur
creation from conflicting amqp interceptors
lukebakken
left a comment
lukebakken
left a comment
There was a problem hiding this comment.
This review was AI-assisted (Claude Code, guided by a human reviewer).
Overall
Clean, well-structured fix. The error propagation chains are correct - we traced each path from rabbit_channel_interceptor:init/1 raising an exit through supervisor:start_child returning {error, _}, through the new case expressions, to either handle_exception (network) or {error, channel_open_failed} (direct). Tests cover both paths with good assertions. A few notes below.
?DEFAULT_TIMEOUT naming
The new macro in rabbit_misc.hrl is very generically named for a public header included across the entire codebase. It's only used by shutdown_supervisor/1. Something like ?SUPERVISOR_SHUTDOWN_TIMEOUT would be clearer and less likely to collide with future uses. Alternatively, define it locally in rabbit_misc.erl rather than the shared header.
5-second timeout vs child shutdown budgets
shutdown_supervisor/1 gives the supervisor 5 seconds to terminate all its children. The children in rabbit_channel_sup have shutdown => ?FAIR_WAIT (70s) and the amqp_channel child in amqp_channel_sup has shutdown => ?WORKER_WAIT (300s). In the specific scenarios this PR addresses, the children being terminated are freshly started healthy processes (limiter, writer) that will exit nearly instantly on a shutdown signal, so 5 seconds is fine in practice. But since shutdown_supervisor/1 is exported and could be called from other contexts, it might be worth either:
- Accepting a timeout argument, or
- Using a value that accommodates the worst-case child shutdown (matching
?FAIR_WAITwould be safe for the server-side supervisors)
Not a blocker - just something to consider for robustness.
|
@Ayanda-D thank you, we will get to polishing this PR a bit in the next week or so. |
|
@lukebakken using |
|
With these changes, certain tests that depended on connection termination due to an exception now fail. The solution is to explicitly handle returned errors in their callers, and in the direct client also explicitly close the connection (on the RabbitMQ end). I am working on the necessary updates. |
`amqp_channels_manager:handle_open_channel/4` now can get an error tuple back where previously connections would fail due to a badmatch, producing crash logs. Note that the broker only actively closes TCP-based connections on these errors (via `rabbit_reader:handle_exception/3`). The direct AMQP 0-9-1 client previously got an identical outcome via the badmatch crash; updating it to properly close is out of scope for this commit and will be investigated next.
|
Merged with additions #16610 for |
3 participants
Proposed Changes
Hello!
We've been noticing that channel creation can fail with
{error, Reason}- for example when channel-interceptors clash on the AMQP operation they intend to intercept (i.e. expected behaviour to fail). The calling processes and supervision tree currently don't seem to handle these errors/failures gracefully, and fail with loudbadmatchexceptions (which are not the most useful for operators). As a result, calling components (connections + supervisors) crash ungracefully. These changes seek to add safe handling of{error, Reason}channel creation errors, to a channel's top/adjacent supervisor, as well as the initiating connection types - both network connections and direct connections. There's alsoshutdown_supervisor/1which ensures clean termination of the related supervisor before an error is returned.CURRENT CRASHES/UNHANDLED CONNECTION TERMINATIONS
Example crash logs (from conflicting channel-interceptor modules/plugins) are as follows:
CRASH-1: rabbit_reader
CRASH-2: rabbit_channel_sup
CRASH-3: rabbit_direct
CRASH-4: amqp_channels_manager
EXPECTED GRACEFUL CONNECTION TERMINATIONS
With these changes, we would only get the relevant failure context and clean termination of the connections:
CLEANER NETWORK CONNECTION TERMINATION
CLEANER DIRECT CONNECTION TERMINATION
( NOTE: These changes should also assist in clean/graceful connection terminations from conflicting channel interceptor failures on same priorities in #16046 )
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.You can also fill these out after creating the PR.
This is simply a reminder of what we are going to look for before merging your code.
CONTRIBUTING.mddocument