search

redis-rb / redis-client

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

Fix BufferedIO to search with `byteindex` #189

Closed casperisfine closed 6 months ago

casperisfine commented 6 months ago

As of https://github.com/redis-rb/redis-client/pull/184, the buffer String is no longer BINARY but UTF-8.

I missed that the code that search for newlines was using .index instead of .byteindex, causing the buffer offset to go out of sync.

casperisfine commented 6 months ago

Arf, byteindex was added in 3.2...

mperham commented 6 months ago

What are the symptoms of this bug? What behavior or errors might the user see?

byroot commented 6 months ago

This bug was never released.

byroot commented 6 months ago

Hum, now that you mention it. I have no evidence it was possible, but if somehow the internal buffer was upgraded to UTF8 when appending bytes, it would lead to a ReadTimeout.

So perhaps, and I insist on the perhaps, it could explain some of the timeout issues reported, but only for people using the Ruby driver. And this is pure conjecture, I'll have a quick look to try to confirm whether this could ever happen before the performance refactor. And if that's the case, this only fixes it for Ruby 3.2+

byroot commented 6 months ago

Hum, so no, I tried various things, and I really don't think it was possible on 0.21.1.

The only two places the buffer is mutated are:

In both case it's handled by read_nonblock.

When passed a buffer, it doesn't change its encoding as long as there is a size provided (https://github.com/ruby/spec/pull/1145)

And when not passed a buffer, similarly, the return string is binary if a size is provided.

I nonetheless simulated this with TCP/UNIX/SSL sockets to double check and couldn't cause any timeout. I also went to audit the OpenSSL gem, and couldn't see any edge case in which it would return a non-binary string.

All this being said, it could be interesting to ask people to experience these issues to log the buffer and offset when it happens.

mperham commented 6 months ago

Yeah, I wondered if it might be the cause of mysterious Sidekiq errors people have been seeing. I missed the recency of the change but this feels like the type of subtle error that can persist (and only trigger very occasionally). Thanks for checking for me.