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 methodto_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.