search

igrigorik / em-synchrony

Fiber aware EventMachine clients and convenience classes
http://www.igvita.com/2010/03/22/untangling-evented-code-with-ruby-fibers
MIT License
1.04k stars 151 forks source link

em-synchrony 1.0.3 and 1.0.4 do not work with ActiveRecord 5.0 #201

Open zdennis opened 8 years ago

zdennis commented 8 years ago

For both em-synchrony 1.0.3 and 1.0.4 I get the same undefined local variable or method 'current_connection_id' error with activerecord 5.0.0.beta1.

/Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/em-synchrony-1.0.3/lib/em-synchrony/activerecord.rb:12:in `block in connection': undefined local variable or method `current_connection_id' for #<ActiveRecord::ConnectionAdapters::ConnectionPool:0x007f9d888a4378> (NameError)
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/em-synchrony-1.0.3/lib/em-synchrony/thread.rb:63:in `synchronize'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/em-synchrony-1.0.3/lib/em-synchrony/activerecord.rb:11:in `connection'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/connection_adapters/abstract/connection_pool.rb:879:in `retrieve_connection'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/connection_handling.rb:113:in `retrieve_connection'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/connection_handling.rb:87:in `connection'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/migration.rb:800:in `connection'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/migration.rb:807:in `block in method_missing'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/migration.rb:786:in `block in say_with_time'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/2.2.0/benchmark.rb:288:in `measure'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/migration.rb:786:in `say_with_time'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/migration.rb:806:in `method_missing'
    from /Users/zdennis/source/opensource_projects/activerecord-import/test/schema/generic_schema.rb:3:in `block in <top (required)>'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/schema.rb:48:in `instance_eval'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/schema.rb:48:in `define'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta1/lib/active_record/schema.rb:44:in `define'
    from /Users/zdennis/source/opensource_projects/activerecord-import/test/schema/generic_schema.rb:1:in `<top (required)>'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-5.0.0.beta1/lib/active_support/dependencies.rb:302:in `require'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-5.0.0.beta1/lib/active_support/dependencies.rb:302:in `block in require'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-5.0.0.beta1/lib/active_support/dependencies.rb:268:in `load_dependency'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-5.0.0.beta1/lib/active_support/dependencies.rb:302:in `require'
    from /Users/zdennis/source/opensource_projects/activerecord-import/test/test_helper.rb:50:in `<top (required)>'
    from /Users/zdennis/source/opensource_projects/activerecord-import/test/import_test.rb:1:in `require'
    from /Users/zdennis/source/opensource_projects/activerecord-import/test/import_test.rb:1:in `<top (required)>'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:15:in `require'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:15:in `block in <main>'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `select'
    from /Users/zdennis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!

I pulled down em-synchrony to add a patch, but after bundling and trying to run the test suite ruby 2.2.3 segfaulted.

Xanders commented 8 years ago

Hello. If you're (or someone) still ineresting in, I've created a branch with ActiveRecord 5 fix: https://github.com/Xanders/em-synchrony/tree/active_record_5 Use it in Gemfile like:

gem 'em-synchrony', github: 'Xanders/em-synchrony', branch: 'active_record_5', require: %w[em-synchrony em-synchrony/mysql2 em-synchrony/activerecord em-synchrony/em-http em-synchrony/fiber_iterator] # just a sample, fill the require list by yourself

Also it looks like tests was fixed two months ago: https://github.com/igrigorik/em-synchrony/commit/1a232d2e74e0ec39f639ef0ae25fbfcb6039329d So project is not totally dead. Hope to see EM-Synchrony support in ActiveRecord-Import in the future again...

dgutov commented 8 years ago

@Xanders Hey, is there a reason you've branched your fix not from master, but from a proposed pull request branch?

Xanders commented 8 years ago

@dgutov Yes, it's because my fix is based on changes from that PR, so we should wait for https://github.com/igrigorik/em-synchrony/pull/190 to be merged until it will be possible to merge my fixes.

dgutov commented 8 years ago

my fix is based on changes from that PR

Any particular reason for that?

190 would have to be rebased first.

Xanders commented 8 years ago

@dgutov Is there any other way to create PR with code using code from other PR except of copy-paste all of that code? I don't understand what the problem now, sorry.

dgutov commented 8 years ago

I'm asking why you've used the approach from that PR and not followed the approach that we currently have in master.

Xanders commented 8 years ago

@dgutov I've tried current master code (both active_record.rb and active_record_4.rb) with AR5 and had a lot of strange errors, didn't know how to fix them. But when i tried fork from #190 it was almost worked except of some moments, that i fixed. I assumed fork is more recent version of synchrony/ar then current master, so I'm forked it, not master. Was it wrong?

dgutov commented 8 years ago

It's not more recent, no. But it's possible that it uses a better approach, considering you've had to fix fewer places.

Xanders commented 8 years ago

@dgutov So I was wrong, sorry. But I think code from that fork is much simpler and more stable, so I still prefer it. But note, I didn't write any heavy/complex tests yet, so some bugs may be there.

dgutov commented 8 years ago

Do the existing tests pass?

Xanders commented 8 years ago

@dgutov Only my app's tests. I didn't run synchrony's tests, I don't have much time. It was just a quick fix to force my app working.

dgutov commented 8 years ago

Please let us know the result when you get around to it.

dgutov commented 8 years ago

Hey folks, could you please try the current em-synchrony master?

See if it works okay with your AR5 based applications.