stompgem / stomp

A ruby gem for sending and receiving messages from a Stomp protocol compliant message queue. Includes: failover logic, ssl support.
http://stomp.github.com
Apache License 2.0
152 stars 80 forks source link

ACKs/NACKs sent with wrong 'id' header when SUBSCRIBE has 'ack' header set to 'client' mode #59

Closed PaulGale closed 11 years ago

PaulGale commented 11 years ago

ActiveMQ 5.8.0 with 1.2.10 of the gem:

In the STOMP log output below, the ACK message has its id header incorrectly populated with the value of the MESSAGE's message-id header. It should, however, populate the id header using the value of the MESSAGE's ack header, given that the ack mode on the subscription is set to client.

As a result ActiveMQ rejects the ACK with the ERROR frame shown.

According to the STOMP spec the same behavior is expected when the ack mode is set to client-individual (not verified).

2013/07/23 22:45:41.869 | TRACE | StompIO                        | Received: 
SUBSCRIBE
content-type:text/plain; charset=UTF-8
activemq.subscriptionName:vicki_event_ods
ack:client
destination:/topic/Vicki.Event
id:vicki_event_processor
content-length:0

2013/07/23 22:46:07.136 | TRACE | StompIO                        | Sending: 
MESSAGE
ack:ID:darkhorse.onyx.ove.com-41603-1374633870794-4:1
message-id:ID:darkhorse.onyx.ove.com-41603-1374633870794-6:1:1:1:1
type:
destination:/topic/Vicki.Event
timestamp:1374633967127
expires:0
subscription:vicki_event_processor
priority:0
correlation-id:

<?xml version="1.0" encoding="utf-8"?>
<univ...universalid>
2013/07/23 22:46:07.354 | TRACE | StompIO                        | Received: 
ACK
content-type:text/plain; charset=UTF-8
id:ID:darkhorse.onyx.ove.com-41603-1374633870794-6:1:1:1:1
content-length:0

2013/07/23 22:46:07.357 | TRACE | StompIO                        | Sending: 
ERROR
content-type:text/plain
message:Unexpected ACK received for message-id [null]

org.apache.activemq.transport.stomp.ProtocolE...d.java:662)
gmallard commented 11 years ago

Accepted. I will post again when I push a fix.

gmallard commented 11 years ago

Acceptance retracted, sorry for the previous.

I can recreate this, but the only way to recreate it is to write incorrect/buggy gem client code.

If a connection is at the 1.2 protocol level, using a Stomp::Connection, and I write:

c.subscribe(q, :ack => "client", :id => "gatest1")
m = c.receive
c.ack m.headers['message-id'] # wrong with 1.2!!

that will cause this failure. But for a 1.2 connection the code should be:

c.subscribe(q, :ack => "client", :id => "gatest1")
m = c.receive
c.ack m.headers['ack']

Right now I am thinking this is probably a bug in your client code. Maybe you want something like:

c.subscribe(q, :ack => "client", :id => "gatest1")
m = c.receive
if c.protocol == Stomp::SPL_12
    c.ack m.headers['ack']
else
    c.ack m.headers['message-id'] 
end

I can not recreate this at all with a Stomp::Client style connection.

Let me know your thoughts please. Right now I do not think this is a problem with the gem code.

PaulGale commented 11 years ago

You are correct. It does not appear to be a bug in the gem.

The Stomp connection is being instantiated and managed via our use of ActiveMessaging v0.12.1.

If you look at the code for ActiveMessaging::Adapters::Stomp::Connection#received in https://github.com/kookster/activemessaging/blob/master/lib/activemessaging/adapters/stomp.rb you can see the bug in their logic.

Thoughts?

gmallard commented 11 years ago

So ....... a couple of points, and finally a suggestion for your problem.

First, if I look at the ActiveMessaging code, I think this is incorrect, starting at line 81:

def received message, headers={}
     #check to see if the ack mode for this subscription is auto or client
     # if the ack mode is client, send an ack
      if (headers[:ack] === 'client')
        ack_headers = message.headers.has_key?(:transaction) ? { :transaction=>message.headers[:transaction]} : {}
        @stomp_connection.ack(message.headers['message-id'], ack_headers) # This is not correct for 1.2 connections
      end
end

It is buggy with a 1.2 connection, which you clearly have. I strongly suggest you open an issue with them. Reference this report if needed.

Second, if you do open an issue with them, please point out to them that they are not using the recommended connection invocation. Their connect is:

          @stomp_connection = ::Stomp::Connection.new(cfg[:login],cfg[:passcode],cfg[:host],cfg[:port].to_i,cfg[:reliable],cfg[:reconnectDelay], connect_headers)

This is not a hashed parameterized connection, which is recommended. See the gem docs for details on that recommendation.

That lack is not a show stopper with this problem, just one of my pet peeves.

Third, the suggestion ... I am guessing that:

this code, line 31:

connect_headers = cfg[:connect_headers] || {}

somehow triggers a CONNECT header like:

accept-version:1.0,1.1,1.2

Of course AMQ 5.8.0 comes back with 1.2. That is per the spec. And it is what triggers your immediate problem.

Can you influence that? Can you somehow influence the 'cfg' hash so that it triggers accepted versions such that they are only:

accept-version:1.0,1.1

I think that should get you through your immediate problem.

As always, let me know your thoughts please.

Many thanks for this report. I actually did not realize that AMQ 5.8 supported (in theory) Stomp 1.2. My test beds have been updated to reflect that.

And the bottom line is: this is not a bug in the stomp gem.

Regards, Guy

gmallard commented 11 years ago

Closing this here. Fix needs to be corrected in ActiveMessaging, per above.