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

Stomp::Client doesn't handle configuration in a thread-safe manner #80

Closed rtyler closed 10 years ago

rtyler commented 10 years ago

We're running multiple threads, each with their own Stomp::Client, sharing a configuration hash, e.g.

config = {:hosts => [{:host => 'localhost', :ssl => false, :port => 61613}, {:host => 'remotehost', :ssl => false, :port => 61613}]}

Thread.new do
  c = Stomp::Client.new(config)
  c.subscribe('/queue/test') do |m|
    puts m.inspect
  end

  loop { sleep 1 }
end

Thread.new do
  c = Stomp::Client.new(config)
  c.subscribe('/queue/test') do |m|
    puts m.inspect
  end

  loop { sleep 1 }
end

loop { sleep 1 }

Because (apparently) of the change_host method inside the client, and because of how Arrays are passed inside of Hashes in Ruby, the second thread will get a mutated hosts parameter.

The work-around for this is to deep-copy the entire config hash, which is unpleasant, e.g.

  c = Stomp::Client.new(Marshal.load(Marshal.dump(config)))
PaulGale commented 10 years ago

You can always add a method on to Hash called deep_copy() that does the marshaling for you.

You cannot avoid the marshaling as the presence of the Logger object requires it. Just implement the appropriate marshaling methods on Slogger.

Having said all of that there's no reason for the change_host method to be modifying the hosts array to begin with.

The fix I proposed, and forwarded to Ian, only required the manipulation a locally held index counter to ensure the correct host was selected from the hosts array. A much cleaner solution than the current implementation of change_host. Why not use that?

gmallard commented 10 years ago

Status: Closed Ref: 49f41b8 a84db0f Gem Version: 1.3.2

If the issue persists please reopen this issue or create a new issue.