redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

Hiredis "Unknown Timeout" #81

Closed yosiat closed 1 year ago

yosiat commented 1 year ago

Hi,

Since upgrading to redis-client from the oldest one, I am starting to get Redis::TimeoutError: Unknown Error while using hiredis-client.

I saw in the documentation:

Contrary to the redis gem, redis-client doesn't protect against concurrent access. To use redis-client in concurrent environments, you MUST use a connection pool, or have one client per Thread or Fiber.

My rails app is running in multi-threaded Puma app, the question - is the comment about concurrency is relevant to hiredis-client as well?

Thanks

casperisfine commented 1 year ago

Please include the full backtrace otherwise it's hard to debug this.

is the comment about concurrency is relevant to hiredis-client as well?

Yes. The simplest is to use a connection pool: https://github.com/redis-rb/redis-client#usage

yosiat commented 1 year ago

The stack is:

/gems/hiredis-client-0.12.0/lib/redis_client/hiredis_connection.rb:74 in _read
/gems/hiredis-client-0.12.0/lib/redis_client/hiredis_connection.rb:74 in read
/gems/redis-client-0.12.0/lib/redis_client/connection_mixin.rb:18 in call
/gems/redis-client-0.12.0/lib/redis_client.rb:225 in block (2 levels) in call_v
/gems/redis-client-0.12.0/lib/redis_client/middlewares.rb:16 in call

But can it be I am getting timeouts, because I am not using connection pool (running in 2 workers, 32 threads) ?

casperisfine commented 1 year ago

Yeah, if 32 threads try to hit the same connection, that's the kind of errors you might see.

yosiat commented 1 year ago

Gotcha, thanks!

Currently I am using Redis.new(..) I'll change it to connection pool suggested from the document, I assume that when I use RedisClient behind the scenes it will use hiredis as driver (if I chosen so)

casperisfine commented 1 year ago

Currently I am using Redis.new(..)

If you are still using redis-rb, you are protected by a Mutex. It's incredibly wasteful because your 32 threads will fight for the lock, but it shouldn't cause errors.

yosiat commented 1 year ago

Gotcha, so I will use ConnectionPool::Wrapper as suggested here - https://github.com/redis/redis-rb#connection-pooling-and-thread-safety

Using the pool directly with RedisClient, requires me to do lots of changes (redis.flushdb -> redis.call("flushdb") for example)

Thanks for your help!

casperisfine commented 1 year ago

Ok, but that's still doesn't explain the error you are seeing. I'd suggest to try without hiredis-client to see if it works, or if it throws a better error.

yosiat commented 1 year ago

I see that we can get "Unknown Error" in multiple cases, what do you about changing it to return "Read timeout" / "Write timeout" error instead of "Unknown Error"?

Think I can do PR for that.

casperisfine commented 1 year ago

I already pushed https://github.com/redis-rb/redis-client/commit/df69b9d8341957c190ed4bbd366f6fe2d0fa7cf1

yosiat commented 1 year ago

You are always so fast 🥇

Umm.. but it can write timeout as well? https://github.com/redis-rb/redis-client/blob/df69b9d8341957c190ed4bbd366f6fe2d0fa7cf1/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c#L669

casperisfine commented 1 year ago

Timeout is in the error class name.

casperisfine commented 1 year ago

To clarify. Ideally I'd like to put a better message, but the first step to do that is to understand what actual error you are actually hitting, and for that I need a repro, or some more info (like the equivalent error without using the hiredis bindings)

yosiat commented 1 year ago

I understand you need a repro, sadly I can't create one - I am getting this timeout from multiple endpoints and different redis operations - it can be simple exists? or mget for multiple keys.

I am abit afraid of moving to pure redis client, because I am not sure what will be it's performance implications and I have no good way of testing it sadly.

My question about the timeout error - if we get timeout in hiredis_read_internal on waiting for writeable, wouldn't it be more correct to throw "write timeout" instead of Unknown Error (hiredis_read) ? Asking this, because from the stack trace -

/gems/hiredis-client-0.12.0/lib/redis_client/hiredis_connection.rb:74 in _read
/gems/hiredis-client-0.12.0/lib/redis_client/hiredis_connection.rb:74 in read

We see it's read throwing a timeout - the question is it write OR read timeout?

byroot commented 1 year ago

I am abit afraid of moving to pure redis client, because I am not sure what will be it's performance implications and I have no good way of testing it sadly.

The difference really isn't that big for most response. It's only on some large array and hash responses that you can really measure it. So really don't be afraid to drop the gem for now.

yosiat commented 1 year ago

We have large responses in our app, anyway I'll do a change next week to have pool & way to change driver, so I can enable with ruby driver and worst case change easily to hiredis.

I'll repo back once I'll have more information, thanks for your help, really appreciated!

byroot commented 1 year ago

No news in a month, closing for now as non actionable. I'll reopen if I get more information

yosiat commented 1 year ago

We eventually did a revert and went back to old redis-rb implementation with hiredis and the timeouts stopped, I have an idea on why it might happen and I'll try to give some more info on this issue.

Sorry for the staleness, thanks.