Improved RabbitMq Compatibility

[fwiesel]

… to rabbitmq and possibly qpid

Apparently, the standard defined incompatible type-codes the pre-existing Qpid/Rabbit extensions,
and at least rabbitmq kept them instead.
Documentation: https://www.rabbitmq.com/amqp-0-9-1-errata.html#section_3
Code: https://github.com/rabbitmq/rabbitmq-server/blob/rabbitmq_v3_5_3/src/rabbit_binary_parser.erl#L41

[fwiesel]

It's a bit of a mess, and I don't think, that is necessarily the way to go.
I am working on having different serialisers/deserialisers for the "dialects".

But that version works for me, and is the minimal implementation to get tables to rabbitmq, which doesn't kill the connection right on registration.

[benjamin-hodgson]

Thanks very much for the pull request, and for your research into the issue: I actually didn't realise this incompatibility between Rabbit and AMQP even existed. What a mess!

As a general rule, I think asynqp should default to whatever RabbitMQ uses (since, really, no one's using anything else), even at the expense of compatibility with the AMQP 0.9.1 specification. Believe me, this situation does not make me happy, and I'm open to advice from anyone who disagrees with me.

I like your idea of having pluggable serialisation code for the various dialects of AMQP. That way we could provide an option to turn on "strict mode" and conform exactly to the spec, while still defaulting to "RabbitMQ mode" in the absence of that option. I guess this would be implemented as a set of classes defining overrides for the various differences between the various dialects. I'd accept a pull request on this issue, though it seems like a fairly big refactoring - if you don't have time to put the work in then I don't mind picking it up, though it might have to wait a week or two before I get the time to put some work in. We'd also have to think about what the API for selecting a dialect might look like.

Regarding the content of this pull request in particular. From reading your linked documentation, I'm not sure that the serialisation code you've included is entirely compatible with RabbitMQ. For example, the documentation states that Rabbit interprets 's' as a signed 16-bit integer, whereas the code suggests that we'd be treating it as a short string, in conformance with the specification. (I realise that it was probably me that wrote that in the first place; I suppose I would've been following the spec, not what was written in the docs you linked.)

So given all that, I'd prefer a pull request which implemented full compatibility with Rabbit; then we can add strict AMQP compatibility back in later as a pluggable option. I also think that perhaps version 0.4 of asynqp shouldn't include this change: I was hoping to put v0.4 out in a couple of weeks (as soon as I've found the time to clean up the error handling API, #23). However if you feel that releasing 0.4 without this change is not an option, I'd like to hear your arguments 😃

[fwiesel]

I am kind of working on a fair a bit of refactoring for allowing such variants. Maybe I should push it to github in a branch, so you can see, what is happening. It is still quite in flux, as I am trying to see, what tools python gives me for that.

The main reason for me not not to include the patch and the argument one would be the incompatibility with other brokers. The current implementation is also not feature complete, so you do not lose much there.

But if rabbitmq is your top-priority, then I would remove the _read_short_string and rather include it, as it enables quite a bit of more functionality.

Why I started all this, was mostly because I needed the auto-delete with ttl on queues, so I can make an application, which can lose the connection and still reconnect to it, without having to implement a bookkeeping on queues, where the client really simply died away.

[benjamin-hodgson]

Yes, I'd be interested to see what your design looks like; I may be able to make some suggestions if you like. On the whole, I'm happy for you to change the serialisation code however you see fit because it's not part of the public API. As long as there's good test coverage I'm happy!

On second thoughts, it looks like the incompatibilities between asynqp and Rabbit (like 's' referring to short strings) that I was worried about are already long-standing issues, so they needn't block this pull request. I've raised #29 to be fixed in the near future.

So, pending your refactorings, do you think this PR is ready to merge?

[fwiesel]

I reworked the code a bit. Now it only follows the rabbitmq code tables, and I added some more field types.
RabbitMq seems to accept the tables and the arrays passed. At least the integration test for custom attributes in #27 shows the list and arrays of integer and strings to come up fine.

[benjamin-hodgson]

@fwiesel GitHub's telling me there are merge conflicts. Please could you rebase your branch onto the latest master and force-push it?

[fwiesel]

Done, sorry for missing that.

[fwiesel]

Sorry, I will fix that quickly.

[benjamin-hodgson]

Looks good 🚢