Add support for ulong header values

[thepunkoff]

Proposed Changes

I added support for having ulongs as header table values for convenience, since uints and ushorts support has already been added.

Types of Changes

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

---

[pivotal-cla]

@thepunkoff Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@thepunkoff Thank you for signing the Contributor License Agreement!

[thepunkoff]

The CI build failed, but I can't see the reasons why. It just says "loading" forever.

[lukebakken]

Thanks!

Build failure:

/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/TestFieldTableFormatting.cs(80,33): error CS1003: Syntax error, ',' expected [/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/Unit.csproj]
/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/TestFieldTableFormattingGeneric.cs(82,33): error CS1003: Syntax error, ',' expected [/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/Unit.csproj]
  Benchmarks -> /tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Benchmarks/bin/Debug/net472/Benchmarks.exe
/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/TestFieldTableFormatting.cs(80,33): error CS1003: Syntax error, ',' expected [/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/Unit.csproj]
/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/TestFieldTableFormattingGeneric.cs(82,33): error CS1003: Syntax error, ',' expected [/tmp/build/b95f7b06/rabbitmq_dotnet_client/projects/Unit/Unit.csproj]

Build FAILED.

I would fix this for you but I'm caught up in customer support!

[thepunkoff]

@lukebakken Thanks for helping! I fixed the build.

[Zerpet]

Actions failed on formatting. Apart from that, this looks good to me. Thank you for your contribution!

[thepunkoff]

There's now a flaky test failure, that prevents this from being merged, but I'm not sure how to rerun the check.

[michaelklishin]

There is one test that fails only on Windows, and the client seemingly goes into a (reconnection) loop that never succeeds:

Testing heartbeats, sleeping for 60 seconds
  Passed RabbitMQ.Client.Unit.TestFloodPublishing.TestMultithreadFloodPublishing [170 ms]
   at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
   at System.IO.BufferedStream.Read(Byte[] array, Int32 offset, Int32 count)
   at RabbitMQ.Client.Impl.InboundFrame.ReadFromStream(Stream reader, Byte[] buffer, Int32 toRead) in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Frame.cs:line 318
   at RabbitMQ.Client.Impl.InboundFrame.ReadFrom(Stream reader, Byte[] frameHeaderBuffer, UInt32 maxMessageSize) in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Frame.cs:line 268
   at RabbitMQ.Client.Impl.SocketFrameHandler.ReadFrame() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\SocketFrameHandler.cs:line 243
   at RabbitMQ.Client.Framing.Impl.Connection.ReceiveLoop() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Connection.Receive.cs:line 73
   at RabbitMQ.Client.Framing.Impl.Connection.MainLoop() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Connection.Receive.cs:line 50
   at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
   at System.IO.BufferedStream.Read(Byte[] array, Int32 offset, Int32 count)
   at RabbitMQ.Client.Impl.InboundFrame.ReadFromStream(Stream reader, Byte[] buffer, Int32 toRead) in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Frame.cs:line 318
   at RabbitMQ.Client.Impl.InboundFrame.ReadFrom(Stream reader, Byte[] frameHeaderBuffer, UInt32 maxMessageSize) in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Frame.cs:line 268
   at RabbitMQ.Client.Impl.SocketFrameHandler.ReadFrame() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\SocketFrameHandler.cs:line 243
   at RabbitMQ.Client.Framing.Impl.Connection.ReceiveLoop() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Connection.Receive.cs:line 73
   at RabbitMQ.Client.Framing.Impl.Connection.MainLoop() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\RabbitMQ.Client\client\impl\Connection.Receive.cs:line 50
   at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)

Anyhow, it does not seem to be related to this PR in any way. The same test passes on Linux.

[lukebakken]

We should at least be compatible with Pika:

https://github.com/pika/pika/blob/main/pika/data.py#L256-L259

PS @thepunkoff thanks for your contribution. If you wouldn't mind making sure this change uses the same single-character as Pika, that would be great.

[thepunkoff]

@lukebakken Ok! I can see that pika uses l for unsigned 64-bit ints and L for signed 64-bit ints. On the other hand the dotnet client lib has been using l for signed ones instead - I'll swap them then to match pika's.

[michaelklishin]

@lukebakken @thepunkoff I don't mind using the same value as Pika but keep this protocol errata in mind. I'd say we should use what the Java client uses and not Pika (in case there is a difference).

[thepunkoff]

@michaelklishin Oh I see now... my bad! So my PR would remove disambiguity between 0-9-1 and RabbitMQ considering longs and ulongs.

However, the Java client currently doesn't support ulongs either. Should I update it as well?

[thepunkoff]

Oops, sorry for messing up commits 😅

[michaelklishin]

@thepunkoff we would appreciate support for the same type in the Java client, of course. To me it appears that Pika uses l as in the original AMQP 0-9-1 spec and not what RabbitMQ and Qpid have adopted.

So both this client and the Java one should the same value, what the RabbitMQ server uses.

[thepunkoff]

Just to clarify. Does the RabbitMQ server use header values as opposed to just passing them along to the consumer to be interpreted by the application? If the server expects values in an exact format, then is it even ok to change anything on the client libraries' side? Should the server be patched to support this, too? How would the server react to such rearrangements in clients?

Considering the errata document. It is stated that there are incompatibilities between amqp 0-9-1 and rmq, I can see those. So does this mean that amqp 0-9-1 clients are not compatible with rmq, and rmq clients are not compatible with 0-9-1-compliant servers? How should I interpret this document? As a todo list or as a rule to follow? Not quite sure yet. Would be glad if y'all clarified it to me :)

[lukebakken]

Here's where the Java client translates the codes to values:

https://github.com/rabbitmq/rabbitmq-java-client/blob/main/src/main/java/com/rabbitmq/client/impl/ValueReader.java#L157-L221

[thepunkoff]

@lukebakken Here's a quote from the errata doc:

RabbitMQ continues to use the tags in the third column.

This actually says the type characters are used. If they're not, and the information is obsolete, then great.

Do you think the field type specifiers in this client should fully conform to the 0-9-1 spec (p. 31-32)? I could make sure they do in this PR, and then update the Java client accordingly. But this should be subsequently done in all the known client libraries.

Also, if an updated client uses new type specifiers, the non-updated clients will be mislead, as e.g. long values in the headers from the updated client would start to use 'L' instead of 'l'.

[Zerpet]

@thepunkoff we would appreciate support for the same type in the Java client, of course. To me it appears that Pika uses l as in the original AMQP 0-9-1 spec and not what RabbitMQ and Qpid have adopted.

So both this client and the Java one should the same value, what the RabbitMQ server uses.

I agree, we should adapt the clients to use what RabbitMQ server uses. At the end of the day, these clients are rabbitmq clients, not pure AMQP 0.9.1 clients.

The Go client seems to follow the same criteria as Java and Pika:

https://github.com/rabbitmq/amqp091-go/blob/5656876376f5139fc349ea903eede73682eff738/read.go#L142-L254

[michaelklishin]

We definitely won't update all clients to use the ambiguous values from the original spec (that were never clarified).

[lukebakken]

Here's where L and l are used in RabbitMQ:
https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbit_common/src/rabbit_binary_parser.erl#L62-L67

I was confusing message headers with the actual framing of messages 🤕

[michaelklishin]

So rabbitmq/rabbitmq-server#1093 (comment) from 2017 suggests we have decided to not support unsigned longs but to support signed longs as effectively all programming languages have a way of representing them on 64-bit machines.

Sounds like we cannot support this type without breaking changes somewhere, and they won't make any sense to Python, JS, PHP, Elixir, Ruby users.

Should we give up on the idea and document the decision in the issue?

[Zerpet]

Should we give up on the idea and document the decision in the issue?

Sounds good to me. I think we should just not have support for unsigned 64-bit integers.

The alternative would be to accept ulong types and encode them, doesn't really matter how i.e. l or L, and let the server interpret them as signed longs. This feels counter-intuitive and could lead to potentially obscure issues. I'd rather just not support them.

[thepunkoff]

Sounds ok to me, too. Just a shame that uints, ushorts and sbytes are already supported, and this all can't be pretty :)

Also, as I can see, at least two clients for languages, that don't have unsigned types (and signed bytes), already will raise an error if they see unsupported values while decoding. Notice the absence of cases for ('u', 'i' ,'b'):

• Ruby
• JavaScript

So the dotnet client can already crash some other clients with these types.