search

iande / onstomp

A STOMP messaging client library for Ruby
http://mathish.com/projects/onstomp.html
Other
23 stars 11 forks source link

ssl options not passed to clients when using failover #26

Open cmbaron opened 9 years ago

cmbaron commented 9 years ago

Options hash including SSL parameters are not passed through to the underlying clients through the failover abstraction.

Proof:

irb> require 'onstomp'
irb> require 'onstomp/failover'

SSL Options passed to Client.new using URI:

irb> client = OnStomp::Client.new('stomp+ssl://activemq:10101', { ssl: { ca_file: 'ca.crt' }} )
irb> client.ssl
=> {:ca_file=>"ca.crt"}

SSL Options passed to Client.new using failover URI:

client = OnStomp::Client.new('failover:(stomp+ssl://activemq:10101)', { ssl: { ca_file: 'ca.crt' }} )
client.client_pool.clients.first.ssl
=> nil
iande commented 9 years ago

First, sorry for taking so long to address this issue.

As it stands right now, the options hash that you pass to the failover abstraction are options specific to failing over, like the retry delay and retry attempts (retry_delay and retry_attempts, respectively.) So, to pass along the SSL options (or any other base client options) you have to use the failover client directly and pass it an array of base clients, like:

client = OnStomp::Failover::Client.new([ OnStomp::Client.new('stomp+ssl://activemq:10101', { ssl: { ca_file: 'ca.crt' } }) ])
client.client_pool.clients.first.ssl
=> {:ca_file=>"ca.crt"}

That is the immediate technical solution to the issue of passing SSL and other options to OnStomp clients when using the Failover client. However, this is interface is unnecessarily complicated. A better option would be to pass along any elements in the option hash that don't pertain to the Failover client to individual OnStomp::Client instances in the pool. And that's pretty much the approach you've taken in your Pull request on this issue. I'm going to merge in your pull request and then make a couple tweaks:

  1. Get the spec cases in place to verify the changes.
  2. Scrub the Failover specific options from the hash before passing it on to the clients in the pool.

I appreciate the time you've put into reporting and patching this issue, and I'll push an updated version as soon as possible.

iande commented 9 years ago

I merged in your pull request with the changes I mentioned above and will get a new version pushed out to rubygems.org shortly. Congrats on being the first "non-me" contributor to the repository and thanks again for the pull request.

I will close this issue when the new OnStomp version has been pushed.

cmbaron commented 9 years ago

Thanks for merging and finishing the test. I was really banging my head on the wall trying to test the that failover client actually passed the options to the pool.

iande commented 9 years ago

No problem, the test was a pain because I got much more "clever" with the code than I should have. There will be a lot of refactoring in the next major release where I support the 1.2 spec changes and remove support for Ruby < 1.9

l8nite commented 6 years ago

Is this in the released library somewhere? I'm running into something similar.