Preserve connection specification on reconnect

[urbanautomaton]

Hi,

In our app we call Vanity.connect! with a connection spec, so that we can use a custom adapter and programmatically determined options, e.g.:

Vanity.connect!(
  adapter: :logging_redis_adapter,
  url: OurApp.config.redis_url
)

However, when connections are established like this, Vanity.reconnect! does not preserve the connection params, and the new connection falls back to the default redis adapter. I've added a failing test to reflect this.

Assuming the intended behaviour is for these values to be preserved, I would be very happy to contribute a fix, but I'd appreciate some feedback on what approach might be best. I notice that currently the collecting option is saved to the configuration on connection. Could this be expanded to save the full connection params? Configuration#connection_params could be modified to be writeable, falling back to params parsed from a file as it currently does.

What do you think?

[phillbaker]

Hi @urbanautomaton, some more context might be helpful here - where does reconnect! get called from?

[urbanautomaton]

Hi Phillip, thanks for the response. In our app, reconnect! gets called in an after-fork hook called by Unicorn when it forks a worker process, so our vanity setup looks like this:

# config/initializers/vanity.rb
Vanity.connect!(
  adapter: :logging_redis_adapter,
  url: OurApp.config.redis_url
)

# config/unicorn.rb
after_fork do |server, worker|
  defined?(Vanity) && Vanity.reconnect!
end

The hook performs basically the same function as the Phusion Passenger hook in the existing Rails integration.

The result is that the master process boots, loading the vanity initializer, which creates the connection correctly. But each forked worker process calls reconnect! and ends up with the default connection spec, { adapter: 'redis' }.

We can work around this with an explicit disconnect!/connect! instead of using reconnect!, but it looks like the Phusion Passenger hook would cause the same issue we experienced.

[phillbaker]

Thanks for the context - I'd probably encourage the workaround of disconnect! / connect! for a couple of reasons:

• the passenger hook is for an outdated version of passenger, I actually meant to remove it in the 1.0 release, but forgot. Going forward the idea would be to keep that complexity out Vanity itself but document common patterns and provide useful APIs - a PR to this effect would be great!
• the collecting option from the connection params is also a bit of a odd duckling - it may be a good configuration parameter, but it's not a connection setting and is considered "legacy".

What do you think about that approach?

[urbanautomaton]

Thanks very much @phillbaker, and apologies for the lack of response until now. 👍