Skip to content

Serialize sequence number as long #1806

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Conversation

lukebakken
Copy link
Collaborator

Fixes issue raised in #1805

@lukebakken lukebakken added this to the 7.1.2 milestone Mar 7, 2025
@lukebakken lukebakken self-assigned this Mar 7, 2025
@lukebakken lukebakken marked this pull request as ready for review March 7, 2025 18:11
@danielmarbach
Copy link
Collaborator

Sorry with fever in bed and can't think straight

@lukebakken
Copy link
Collaborator Author

@danielmarbach no worries, get well soon!

Some more context in this comment of mine:
gftea/amqprs#158 (comment)

@zlepper
Copy link

zlepper commented Mar 7, 2025

Out of curiosity, wouldn't it be better to actually encode the header as the number type directly, rather than going through a string? I believe headers do support numbers as value directly (at least I recall banging my head against that for a Rebus.rabbitmq bug a while ago)

Though I guess allocation-wise it won't matter much as it's either a string allocation or a boxing allocation?

I'm asking mainly to learn, as there might be some delicious knowledge to be had :)

Either way thank you for the extremely quick response time on the issue!

@lukebakken
Copy link
Collaborator Author

wouldn't it be better to actually encode the header as the number type directly

Yep, it would be, but RabbitMQ and QPid stray from the AMQP 0.9.1 spec when it comes to unsigned long long values:

https://www.rabbitmq.com/amqp-0-9-1-errata

Notice that the spec specifies that unsigned long long should be encoded with l:
image

...but RabbitMQ does not work that way:

image

@lukebakken lukebakken force-pushed the rabbitmq-dotnet-client-1805 branch from 882980d to 4506de7 Compare March 7, 2025 22:46
@lukebakken
Copy link
Collaborator Author

@zlepper your suggestion made me consider an alternative.

  • If the publish sequence number is less than long.MaxValue, use the long directly in the header.
  • Else, encode as an ASCII string

Considering that it would take publishing one message per millisecond over the next 292,471,208 years to exceed long.MaxValue, I think this is a good solution 😉

@lukebakken lukebakken changed the title Serialize sequence number as string. Serialize sequence number as long Mar 7, 2025
Fixes issue raised in #1805

Encode publish sequence number in x-dotnet-pub-seq-no as a `long` instead of ASCII string, unless value is greater than `long.MaxValue`.
@lukebakken lukebakken force-pushed the rabbitmq-dotnet-client-1805 branch from 4506de7 to b057ca2 Compare March 7, 2025 22:52
@paulomorgado
Copy link
Contributor

@zlepper your suggestion made me consider an alternative.

  • If the publish sequence number is less than long.MaxValue, use the long directly in the header.
  • Else, encode as an ASCII string

Considering that it would take publishing one message per millisecond over the next 292,471,208 years to exceed long.MaxValue, I think this is a good solution 😉

There's always BigInteger. 😄

@lukebakken lukebakken merged commit 24a3772 into main Mar 11, 2025
16 checks passed
@lukebakken lukebakken deleted the rabbitmq-dotnet-client-1805 branch March 11, 2025 13:37
@AlfonsRoerdink
Copy link

AlfonsRoerdink commented Mar 11, 2025

Just tested this master branch locally and it seems to be a much better solution. In our case a SAP pi po rest adapter is having a lot of trouble with the previous value of the x-dotnet-pub-seq-no header while transforming it to XML.

@lukebakken any idea when a new (alpha) release will be made?

Many thanks for this fix, a saved us from a big headache.

@lukebakken
Copy link
Collaborator Author

any idea when a new (alpha) release will be made?

As soon as I can fix this issue - #1802

It is difficult to reason about.

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.

5 participants