Support ActiveRecord 5.0

[hyoshida]

I was using em-synchrony in Rails 4.2. It works perfectly.

But em-synchrony couldn't be running after update Rails to 5.0. The error is something like this:

undefined local variable or method `current_connection_id' for #<ActiveRecord::ConnectionAdapters::ConnectionPool:0x000008135f5c38>
/usr/home/hyoshida/src/sample/vendor/bundle/ruby/2.3.0/gems/em-synchrony-1.0.5/lib/em-synchrony/activerecord.rb:12:in `block in connection'
/usr/home/hyoshida/src/sample/vendor/bundle/ruby/2.3.0/gems/em-synchrony-1.0.5/lib/em-synchrony/thread.rb:63:in `synchronize'
/usr/home/hyoshida/src/sample/vendor/bundle/ruby/2.3.0/gems/em-synchrony-1.0.5/lib/em-synchrony/activerecord.rb:11:in `connection'

I think the cause is the following change in ActiveRecord:

# ActiveRecord::ConnectionAdapters::ConnectionPool#connection
AR 4.2: @reserved_connections[current_connection_id] ||= checkout
AR 5.0: @thread_cached_conns[connection_cache_key(Thread.current)] ||= checkout

I tried to fix this error. Hope you like it.

[dgutov]

The definition of connection in 5.0 is as simple as you say, but in 4.2 it also has a synchronize call.

Looks like this change will add some overhead under 4.2 which the current code tries to avoid.

[hyoshida]

Thank you for the reply, @dgutov.

Indeed, My change will add overhead under Rails 4.2.
I wrote a monkey patch as a workaround.

By the way, I think em-synchrony does not need ActiveRecord's thread-safe implementation, Because ActiveRecord on em-synchrony is always working with single thread. It's for this reason that em-synchrony will remove synchronize for Thread, right?
Please tell me if I'm wrong.

[dgutov]

@hyoshida Could we move the version check to above the level of def connection? And define the function one or the other way depending on the result.

By the way, I think em-synchrony does not need ActiveRecord's thread-safe implementation, Because ActiveRecord on em-synchrony is always working with single thread. It's for this reason that em-synchrony will remove synchronize for Thread, right?

I... guess? Are you proposing some further change?

[hyoshida]

@hyoshida Could we move the version check to above the level of def connection? And define the function one or the other way depending on the result.

OK, Fixed it.

Are you proposing some further change?

No. I just wanted to confirm.

[dgutov]

No. I just wanted to confirm.

OK. If I understand your question right, it's not important that we only use one thread. Because we synchronize on a "fibered mutex" instead, and it's more granular than synchronizing on a thread.

[dgutov]

Thanks!

[hyoshida]

OK. If I understand your question right, it's not important that we only use one thread. Because we synchronize on a "fibered mutex" instead, and it's more granular than synchronizing on a thread.

I see.

Thank you for the answer and merge :)