Detected race condition.

[marshall-lee]

This one demonstrates that celluloid-redis is unsafe under a heavy concurrent usage.

There is also related issue: celluloid/celluloid-io#151 - besides this race condition it also introduces some strange behavior of Celluloid::Mailbox::Evented that I tried to fix in celluloid/celluloid#666

[marshall-lee]

This one also forced me to fix #17 because solution to this issue is to 'duck-type' Celluloid::Redis constant (now it's a class inherited from ::Redis)

[marshall-lee]

And as a new class Celluloid::Redis appeared, there's no need to rename a gem to something like celluloid-redis-connection as we discussed in #18 :)

[marshall-lee]

But don't merge for the time being!
It doesn't work well with subscribe which requires re-enterable mutex :(

[tarcieri]

@marshall-lee I think having both ::Redis and Celluloid::Redis where one is a subclass of the other is really confusing

[marshall-lee]

@tarcieri what'd you suggest? delegation?

[tarcieri]

I would suggest renaming the gem and changing the API so there aren't any conflicting names

[marshall-lee]

Inclusion of Celluloid::IO::TCPServer is just the same thing, isn't it?

[marshall-lee]

Why here this approach is bad?

[marshall-lee]

I know you want this gem to be just a driver. But I don't know how to fix a synchronization issue without intercepting something in Redis. It requires this gem to be more than just driver. So I just applied an approach celluloid-io users are already familiar with. I don't understand why it's confusing. It's as confusing as TCPServer and TCPSocket inclusion.

[tarcieri]

Inclusion of Celluloid::IO::TCPServer is just the same thing, isn't it?

No. Keeping Redis in the Celluloid namespace opts anything that includes Celluloid and also uses Redis into using the Redis Celluloid::IO driver, even if it's not using Celluloid::IO, which defeats the point.

If the class were called Celluloid::IO::Redis it'd be a different story, but that's not the case.

[marshall-lee]

@tarcieri

anything that includes Celluloid and also uses Redis into using the Redis Celluloid::IO driver, even if it's not using Celluloid::IO, which defeats the point.

This is not true. Look at the implementation — it doesn't touch Redis behavior if driver is not :celluloid.

But I can improve it by moving into Celluloid::IO namespace and even define Celluloid::IO::Redis.new method that fairly returns ::Redis instance if driver is not :celluloid.

What do you think?

[tarcieri]

Moving it into the Celluloid::IO namespace makes sense to me. Beyond that I don't really care so long as it's completely compatible.

[marshall-lee]

BTW I realized what this race condition exactly is. Running new spec without synchronize produces this picture in /var/log/redis/redis-server.log (with loglevel debug setting):

[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56302
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56303
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56304
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56305
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56306
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56307
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56308
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56309
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56310
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56311
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56312
[12272] 24 Aug 12:28:31.175 - Accepted 127.0.0.1:56313
...

For every command Redis creates a new connection. It's because of implementation of ensure_connected - it reconnects if @pending_reads > 0. This is why reading from closed socket (#<IOError: closed stream>) is possible.
Even if it's not the real reason at least redis-rb is simply not designed for concurrent operations :(