kickstarter/slave_pools — pull request #7 by cainlevy (2014-01-14 22:10:56+00:00)
Title: thread bugs

The recent refactor introduced thread variables, and so we have our first thread bugs!

The NewRelic RPM agent creates another thread, sleeps, and then harvests/samples some data. When run with the Delayed::Job dispatcher, it samples the `delayed_jobs` table to determine the number of failed jobs, number of locked jobs, and total queue size. This executes a query on the database, and when it happens for the first time, SlavePools has to recreate the `SlavePools.pools` thread var.

This revealed two bugs:

1. In rare cases, `SlavePools::Pools#valid_connection?` was returning false in the new thread. This left the proxy without any pools, causing a `NoMethodError`. This was easily fixed by moving the create-fallback-pool-with-master logic where it could be evaluated on demand.

2. More significantly: `SlavePools::Pools#valid_connection?` _was not threadsafe_. It was checking the validity of a connection spec by swapping `ActiveRecord::Base.connection`, which is not a thread var. If the process scheduler switched back to the Delayed::Worker's thread in the middle of the agent thread's connection test and tried to execute or finish a query, the worker thread's connection would be mysteriously missing/modified/corrupted.

Rather than fix `#valid_connection?` to be threadsafe, I'm opting to remove it. It no longer fits with our philosophy of placing the burden to provision healthy databases on chef, and could've resulted in processes that unexpectedly had no replicas and sent all their traffic to master.