amqp
amqp copied to clipboard
streadway
Unhandled error, publishing to an exchange using an invalid exchange type
Hi there,
I seemed to have been getting panic when writing to a closed (go not rabbit) channel, which completely makes sense. However, I got in this state due to a typo in my exchange type, x-consistent-hash was spelled x-constistent-hash. I think I see a couple ways this error state can be handled gracefully, if this change is welcomed, I'd be more than happy to create a pull request for this. Returning an error for this on declare of the exchange seems to make the most sense to me, but I'm certainly open to discussing.
This https://github.com/streadway/amqp/pull/196, seems related to what happens when this isn't caught.
Specifying an incorrect exchange type will result in an unrecoverable (connection) exception. What kind of improvements do you have in mind?
#196 does look relevant but I don't remember what the decision was (if any). Maybe @gerhard or @streadway do :)
I'm certainly open to suggestions if this is not ideal, but you could validate the exchange type, possibly using a map[string]bool, where the keys are valid exchange types, though I can understand if that's not ideal due to the need to keep it up to date every time valid exchange types are either removed or added.
Exchange types can be provided by plugins. We cannot limit types to the list of built-in or known ones.
figured so, hmm, I'll have to look into what happens when declaring an exchange of a wrong type a little further then.
@cpg1111 it would be much easier to suggest something if you could post your code. The issue is more about connection.close handling than exchanges or exchange types. In fact, I wouldn't be surprised if it's a different manifestation of #196.
Yes it definitely is https://github.com/streadway/amqp/pull/196 that causes the panic, tracing my code shows that. My issue is more about handling my cause of that beforehand. I am saying handle the wrong exchange type, not the race condition (in this issue specifically). Here's my code:
for <condition arbitrary> {
go func(ex, exType string, c *amqp.Conn){
var err error
channel := c.Channel()
err = channel.ExchangeDeclare(ex, exType, true, false, false, false, nil)
if err != nil {
log.Error(err)
return
}
err = channel.Publish(ex, exT, "comeFromConfig", true, false, amqp.Publishing{
ContentType: "someMSGType",
ContentEncoding: "utf-8",
DeliveryMode: 2,
Body: <some body>,
})
if err != nil {
log.Error(err)
}
}("exchange", "x-consistent-hash", conn) // originally mispelled --> "x-constistent-hash"
}
@cpg1111 how can the library know what exchange type is "wrong" if it can be any string that begins with an x-?
Some other clients, e.g. Bunny, have helpers for known exchange types: topic, fanout, direct, etc. We could introduce additional functions that declare exchanges of that type but I have a feeling @streadway won't be happy about the explosion of functions with more or less identical purposes.
Another thing we can do is to introduce constants for known exchange types. I think this should be an easier sell.
I do like the constants idea personally. I was thinking of handling a failed attempt after it's sent to rabbitmq in Channel's sendOpen function, but after further inspection, I see you won't get an explicit error from rabbit of a bad exchange type.
@cpg1111 constants is an easy and no risk solution. @gustamora sorry, can you elaborate?
@cpg1111 we believe registering an error handler on the connection is what you want. @streadway @gerhard and I agree that introducing constants for known exchange types is a good idea.
@michaelklishin yeah I've come to that inclusion, thanks for pointing me there. And yeah I think the constants would would be a great idea.