Should Connection#pipeline have the same retry logic as #run and #multi?

[robacarp]

Honeybadger captured an error recently for me which was caused by a connection blip between the mosquito worker and Redis. It could have been a transient network failure or something else, but the exception thrown was:

Redis::Pipeline::ResolutionError: Failed reading pipeline item 0: "Connection closed"

bin/worker in 'raise<Redis::Pipeline::ResolutionError>:NoReturn':0 in **unknown**
bin/worker in 'Redis::Pipeline#commit:Array(Array(Redis::Value) | Int64 | Redis::Error+ | String | Nil)':0 in **unknown**
bin/worker in 'Mosquito::RedisBackend::Queue#enqueue<Mosquito::JobRun>:Mosquito::JobRun':0 in **unknown**
bin/worker in 'Mosquito::Queue#enqueue<Mosquito::JobRun>:Mosquito::JobRun':0 in **unknown**

That led me to some investigating about what Mosquito should do in this situation. What happened is that the mosquito worker crashed and was rebooted. That's maybe the best case, but the error message was initially opaque to me.

Anyway, connection run and multi both have similar logic but pipeline does not. I thought that might be by design, but I'm unsure. It seems almost identically risky to try to retry a multi as a pipeline.

Connection#pipeline

    def pipeline(&)
      pipeline = Pipeline.new(self)
      error = nil
      begin
        yield pipeline
      rescue ex
        error = ex
      end

      flush
      result = pipeline.commit

      if error
        raise error
      else
        result
      end
    end

Connection#run

    def run(command, retries = 5) : Value
      start = instant_time

      loop do
        @writer.encode command
        flush
        return read
      rescue ex : IO::Error
        if retries > 0
          retries -= 1
          initialize @uri
        else
          raise ex
        end
     # ...

Connection#multi

    def multi(retries = 5, &)
      loop do
        txn = Transaction.new(self)

        begin
          txn.start!
          yield txn
        rescue ex
          txn.discard
          raise ex
        end
      rescue ex : IO::Error
        if retries > 0
          retries -= 1
          initialize @uri
   # ...

[jgaskins]

If pipeline doesn't have the same retries as the other methods, that's definitely not intentional. I added retries to run and multi as a result of issues I was seeing in production and I probably just overlooked pipeline.

In fact, multi and pipeline should have nearly identical implementations because they're nearly identical conceptually — both enqueue work and defer returning the results. The main difference between them is how the server processes that work. pipeline does need to trap the error the way it does to re-raise later, though, because it's not atomic and needs to read the results of a partially completed block. I feel like they should be identical otherwise, though.

[dup2]

We just had the same kind of errors repeatedly under high load when using the default settings (no custom pool size or keepalive settings): Failed reading pipeline item 0: \"Connection closed\"

Strange thing is tough that we do not use pipelines at all, the error was raised when using @redis_client.ping in a health check endpoint.

[jgaskins]

@dup2 Could that be the auth/slot selection pipeline on initial connection?

[dup2]

Yes, that sounds right and is a very good pointer, was not aware that this uses pipelining.

Is there any workaround for this in the connection / pooling configuration. I'd probably try keepalive settings, retry and pool sizing to get this improved without code changes.

[jgaskins]

Is there any workaround for this in the connection / pooling configuration

I don't think so. That happens during connection initialization, so the connection should be fresh. Maybe it's happening somewhere else, but I don't know off the top of my head where else we use pipelining internally.

I have a PR open to the db shard to add a before_checkout hook where we could implement periodic health checks that, if they fail, would cause the pool to discard the connection and check out a different one. This could help fix the issue you're seeing.