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

JBEAP-6170 messages could be sent to wrong queue #103

Merged
merged 1 commit into from Dec 20, 2016

Conversation

treblereel
Copy link

@treblereel treblereel commented Sep 8, 2016

In rare circumstances MessageProducer can send a message
to wrong queue

JIRA: https://issues.apache.org/jira/browse/ARTEMIS-715
https://issues.jboss.org/browse/JBEAP-6170

(cherry picked from commit 62f90ea)

@dudaerich
Copy link

Hi, I reviewed your change and I think that it is not correct. Imagine a following scenario:

MessageProducer producer = session.createProducer(queueA);
producer.send(queueB, msg);
  1. Producer is created with default queue queueA => ClientProducerImpl.address = queueA
  2. Producer send message to queueB => ClientProducerImpl.doSend(queueB, msg, ...)
  3. In method ClientProducerImpl.sendRegularMessage
    1. session.checkDefaultAddress(address) => default address on client side is queueA
    2. the very first message is sent to queueB so server set the default address to queueB
    3. default addresses on client and server are different => error

In the original fix on HornetQ the sendingAddress from doSend method is passed to session.checkDefaultAddress not ClientProducerImpl.address.

@andytaylor
Copy link

@dudaerich are you saying the upstream fix is wrong or it has been cherry picked incorrectly?

@andytaylor
Copy link

Also the EAP Jira needs to be added to the commit mesage

@dudaerich
Copy link

@andytaylor the upstream fix is wrong as well.

@treblereel
Copy link
Author

@dudaerich my fault, is it better ?

@andytaylor
Copy link

Then it needs fixing upstream and both fixes cherry-picked

@dudaerich
Copy link

@treblereel yes, it looks good now.

@treblereel
Copy link
Author

@dudaerich done, apache#781

@treblereel
Copy link
Author

retest this please

@dudaerich
Copy link

Tests passed.

@mtaylor
Copy link

mtaylor commented Sep 28, 2016

@treblereel can you please ensure that you have included the upstream JIRA, the cherry-pick commit and the downstream JBEAP-XXXX. In the "commit message" not just the PR message.

The JBEAP referenced in the PR has been closed. Is this the correct one: https://issues.jboss.org/browse/JBEAP-6170

@treblereel
Copy link
Author

@mtaylor today is a public holiday, may i check it tomorrow ?

@mtaylor
Copy link

mtaylor commented Sep 28, 2016

@treblereel yes of course. Enjoy!

@treblereel
Copy link
Author

treblereel commented Sep 30, 2016

@mtaylor sorry for delay, i think, it happens, because i did this PR before QA did JBEAP-6065 (new scenario), maybe it's better to change reference from JBEAP-4721 to JBEAP-6065, is case 4721 already verified.

ps. i have to check which branch is used for 7.1 ...

@wolfc
Copy link

wolfc commented Nov 14, 2016

It should be enough to have all links correctly in the PR description. Retro-fitting commit messages is very much a pain.

  • JBEAP-6065 is a 7.1.0 issue. This needs to be linked to a 7.0.x issue.

@treblereel treblereel changed the title ARTEMIS-715 messages could be sent to wrong queue JBEAP-6170 messages could be sent to wrong queue Nov 21, 2016
In rare circumstances MessageProducer can send a message
to wrong queue
@mtaylor
Copy link

mtaylor commented Dec 19, 2016

@wolfc I don't know what you mean re: "retro fitting a commit message is very much a pain". During the cherry-pick just add the JBEAP JIRA and information required. To fix it (if someone forgot) git rebase -i and a rename. The whole process takes less that a minute and it makes it easier for us to track which commits align to particular JIRAs.

@mtaylor mtaylor merged commit 0cc4395 into rh-messaging:jboss-1.1.0-x Dec 20, 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
5 participants