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

should not by default allow duplicate subscribes #21

Closed ripienaar closed 12 years ago

ripienaar commented 12 years ago

Hello,

I think the behavior with duplicate subscribes is confusing and / or broken.

client.subscribe("/topic/foo")
client.subscribe("/topic/foo")

client.publish("/topic/foo", "hello")
p client.receive.body
p client.receive.body

This gives you two copies of the same message. Now at the most basic I think you should not be able to subscribe twice to the same source without specifically enabling some flag to force this.

Additionally you store subscriptions in @subscriptions and on a later connection attempt you replay the subscriptions this is fine but in the case of dupe subscriptions you will loose the duplicated subscription.

So if you ran the code above followed by:

client.disconnect
client.publish("/topic/foo", "hello")
p client.receive.body
p client.receive.body

You'd now only receive 1 copy of the message as the dupe subscription isnt replayed.

gmallard commented 12 years ago

I am going to close this at some point, but please open separate issues for each of the points you mention if you so choose.

You use an instance variable named 'client'. But this is really a:

Stomp::Connection

is it not? The

Stomp::Client

does not have a method named 'disconnect'.

Please try to make it clear which of the two interfaces you are using.

Before you open an issue for the first point, I suggest you think about the usual server side semantic differences between 'topics' and 'queues'. The example you show is absolutely not surprising to me for 'topics'. Change that to a queue and try it.

Furthermore, in the first point you do not even know where the two messages came 'from'. Why? Because you do not subscribe with a unique id for each subscription ID (parameter 3 on the subscribe). It is your responsibility to do that.

ripienaar commented 12 years ago

Indeed, I use Stomp::Connection sorry for not being clear.

I would expect not to be able to re-subscribe to the same topic twice either by accident - for a queue this would be fine because you would still only get a message once but for topics this will be a multiplication effect which isnt good and should only be allowed when forced via a specific flag

I'll open other tickets sure.

gmallard commented 12 years ago

On your second point: yes this is a bug, but I expect is is not exactly what you think.

The replay mechanism is meant to come in to play with reliable connections after a successful reconnect to the server.

In your example, you have called 'disconnect'. You are done, finished. You just said that.

We should not allow any further calls using that connection instance, and should (I am currently thinking) raise an exception.

gmallard commented 12 years ago

We'll do something about both of these. They are on the todo list.

For point number 1, a couple of notes: First, we are sloppy about this because the STOMP 1.0 spec is sloppy about it. This is tightened up a lot in the 1.1 spec, and indeed your code would fail in some way.

Second: if you were using the 'client' interface, rather than the 'connection' interface you would get what you ask for. A 'connection' is more low level, and the expectation is that the application implementer take a little more care with design. We do not work too hard at preventing you from shooting yourself in the foot. For example, this:

client = Stomp::Client::new("acli", "apw", "localhost", 61613, true)
dest = "/queue/foo"
message1 = nil
client.subscribe(dest) {|m|
    p m.body
    message1 = m
}
client.publish(dest, "hello1")
sleep 0.1 until message1
message2 = nil
client.subscribe(dest) {|m|
    p m.body
    message2 = m
}
sleep 0.1 until message2
client.close

is going to get you something like this:

$ ruby stompsimple.rb
"hello1"
/opt/r193_20110731/lib/ruby/gems/1.9.1/gems/stomp-1.1.9/lib/stomp/client.rb:150:in `subscribe': attempting to subscribe to a queue with a previous subscription (RuntimeError)
    from stompsimple.rb:28:in `<main>'

We'll tighten that up for a 'connection'. A small step toward 1.1.

For point 2: well, that 'publish' after 'disconnect' is going to go KABOOM.

ripienaar commented 12 years ago

Awesome, this sounds great thanks.

I am not a big fan of the style of programming with the client class, prefer the connection but thats a fair point about it being more low level.

Happy with the 2nd case going boom :)

Do you still want me to make 2 tickets?

gmallard commented 12 years ago

This ticket will be fine, thanks.

gmallard commented 12 years ago

Closed. Scheduled for 1.1.11.