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

Fix queue declaration arguments #50

Merged
merged 2 commits into from
Sep 22, 2016
Merged

Fix queue declaration arguments #50

merged 2 commits into from
Sep 22, 2016

Conversation

TomVS
Copy link

@TomVS TomVS commented Jun 22, 2016

Hi,

Thanks for your work.
I encountered a bug when trying to declare a queue with x-max-priority = 4 set in optional arguments. The argument set didn't have any effect.
I fixed the issue in the code (variable name clash).

Best regards,
Thomas

@mbroadst
Copy link
Owner

mbroadst commented Jun 24, 2016

@TomVS this arguments => args seems really fishy to me, can you explain what happened again? You're saying there was an arguments clash, but a compiler definitely would have picked that up.

Also, can you provide an example where this failed? It would be best to include a test to make sure we don't experience this again.

@TomVS
Copy link
Author

TomVS commented Jun 24, 2016

Hi,

I realized I pushed another commit to my master for the MessageId property. I can remove that as it is not relevant to the bug fix I want pull into your master. (commit 2270f0b)
While I'm on that topic, what is your reasoning for setting the MessageId to 0? Does it have to be set at all before publishing a message?

The issue with that arguments variable was that you were writing to the QByteArray arguments through the QDataStream out, then writing the QByteArray arguments local variable into the out stream instead of the QAmqpTable arguments from the QAmqpChannelPrivate class.
The local variable was being used instead of the class variable.
There are two easy ways of fixing this that I thought of:

  • rename the local variable (what I did), or
  • use this->arguments to make sure we are writing the actual arguments into the frame.

A simple example:

...
    m_client->createQueue("queue-key");
    QAmqpTable args;
    args.insert("x-max-priority", 4);
    tmpQueue->declare(QAmqpQueue::Durable, args);
...

The queue declared and created with this code does not have any x-max-priority property.
With my bug fix, the priority queue is declared correctly.

Let me know what you think. I shall look into writing a test for this.

@mbroadst
Copy link
Owner

wow thats totally bizarre that the compiler allowed the variable to be named the same thing, never heard of such a thing. okay sounds good, thanks for the fix! I think it should be incredibly easy to copy one of the existing tests to prove that this declared properly if you don't mind taking the time.

Also, the messageId was a holdover from the original implementation by fuCtor, so no particular reason that its hard coded to 0. We can keep that in this PR, that's fine

@mbroadst mbroadst merged commit 6d73c4c into mbroadst:master Sep 22, 2016
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