Vanity.playground.ab_assigned throws NoMethodError if Redis connection is down

[kirhgoff]

While testing Vanity (2.2.7) behavior in situation when Redis is down, we discovered that the following code Vanity.playground.participant_info(vanity_identity) throws an error NoMethodError (undefined method to_i' for true:TrueClass):` with no stack trace in Vanity code.

We traced how the error and found a workaround, but it probably makes sense to get fixed on Vanity side.

1. Here is Vanity.playground.participant_info code:

    def participant_info(participant_id)
      participant_array = []
      experiments.values.sort_by(&:name).each do |e|
        index = connection.ab_assigned(e.id, participant_id)
        if index
          participant_array << [e, e.alternatives[index.to_i]]
        end
      end
      participant_array
    end

Looks like when the connection to Redis is down, ab_assigned returns true instead of nil or 0, which is being converted to int

2. This is the code of RedisAdapter.ab_assigned

      def ab_assigned(experiment, identity)
        call_redis_with_failover do
          Vanity.playground.experiments[experiment].alternatives.each do |alternative|
            if @experiments.sismember "#{experiment}:alts:#{alternative.id}:participants", identity
              return alternative.id
            end
          end
          nil
        end
      end

So it either return alternative.id or nil. But we get true, how come? Lets check call_redis_with_failover method

3. This is RedisAdapter.call_redis_with_failover

     def call_redis_with_failover(*arguments)
        calling_method = caller[0][/`.*'/][1..-2]
        begin
          yield
        rescue => e
          if Vanity.configuration.failover_on_datastore_error
            Vanity.configuration.on_datastore_error.call(e, self.class, calling_method, arguments)
          else
            raise e
          end
        end
      end

It either rethrows exception, or return blocks return value, or... it returns result of on_datastore_error callback.

4. This is the callback we have (documentation says that return value will be ignored, but looks like it is not http://www.rubydoc.info/gems/vanity/Vanity/Configuration#on_datastore_error=-instance_method)

Vanity.configure do |config|
  config.failover_on_datastore_error = true
  config.on_datastore_error =  Proc.new do |error, klass, method, arguments|
    Rails.logger.error("Vanity error: #{error}. Called by #{klass.inspect}, method #{method.inspect} with arguments #{arguments}")
  end
end

5. If we add nil as return value of on_datastore_error callback, everything works ok. Without it, Rails.logger.error seems to return true which is being returned from ab_assigned and breaks participants_info method logic.

We are not sure how it should be fixed, probably documentation just should mention returning nil.

[phillbaker]

@kirhgoff thanks for the very detailed issue!

I wonder if we should instead be explicitly returning nil from call_redis_with_failover in the case of an exception? For example,

 def call_redis_with_failover(*arguments)
    calling_method = caller[0][/`.*'/][1..-2]
    begin
      yield
    rescue => e
      if Vanity.configuration.failover_on_datastore_error
        Vanity.configuration.on_datastore_error.call(e, self.class, calling_method, arguments)
        return nil # don't implicitly return the value returned by `on_datastore_error`
      else
        raise e
      end
    end
  end

[kirhgoff]

Another approach is to have a default_result parameter with default value as nil to give other methods calling call_redis_with_failover possibility to provide default return value to have more flexibility (some method would like to have [] or {} to be returned instead of nil). Though not sure whether this is needed and probably simple return nil would suffice.