Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delivery_info to properties for error handler #395

Merged
merged 3 commits into from
May 7, 2024

Conversation

tlloydthwaites
Copy link
Contributor

When handling an error, it would be useful to know the delivery_info such as the routing key. This adds delivery_info to message properties. This seemed the only way to supply the information without changing the error handler contract.

When handling an error, it would be useful to know the delivery_info such as the routing key. This adds delivery_info to message properties. This seemed the only way to supply the information without changing the error handler contract.
@michaelklishin
Copy link
Member

Message properties and delivery_info are not the same thing (semantically in the protocol and in Bunny). For that reason, I don't like merging them together.

I need to take a look at what other options we may have, although very likely that'd mean modifying the error reporting interface :(

@tlloydthwaites
Copy link
Contributor Author

tlloydthwaites commented Jul 4, 2023

Yep, it's a hack... I don't see any other option other than updating the interface.

One solution might be to check arity on the handler method to see if it allows the extra param.

Something like:

handle_error(properties, payload, consumer, ex, delivery_info)

then:

backend.handle *args.first(backend.method(:handle).arity)

This relies on delivery_info being the last parameter in the handler. We could also check using something like:

if backend.method(:handle).parameters.map { |e| e[1] }.include?(:delivery_info)

But the arity method would allow the extra param on the end without disrupting existing handlers.

Allows method handlers to specify delivery_info as the 5th parameter. Existing handlers will continue to only receive 4 parameters.
@tlloydthwaites
Copy link
Contributor Author

Updated PR with arity check, working nicely here :)

@michaelklishin
Copy link
Member

I still don't find the merge of the two properties to be a good idea but for error reporting and a set of existing error handlers, I guess, this may be the most pragmatic option 😭

@michaelklishin michaelklishin merged commit 3591e5f into ruby-amqp:main May 7, 2024
@sharshenov sharshenov mentioned this pull request Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants