Simple use of EM::Synchrony#sync causes 'root fiber' FiberError -- my fault? #103

Closed
blt opened this Issue Jan 5, 2012 · 13 comments

Comments

Projects
None yet
3 participants

blt commented Jan 5, 2012

This program

require 'em-synchrony' ## v1.0.0                                                                                                                               
require 'em-hiredis'   ## v0.1.0                                                                                                                               

module EventMachine
  module Hiredis
    class Client

      def self.connect(host = 'localhost', port = 6379)
        conn = new(host, port)
        EM::Synchrony.sync conn.connect
    conn
      end

      alias :old_method_missing :method_missing
      def method_missing(sym, *args)
        EM::Synchrony.sync old_method_missing(sym, *args)
      end
    end
  end
end

EventMachine.synchrony do
  redis = EM::Hiredis.connect

  redis.set('foo', 'bar')
  puts redis.get('foo')

  EM.stop
end

dies like this


$ ruby /tmp/reddy.rb 
/home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-synchrony-1.0.0/lib/em-synchrony.rb:58:in `yield': can't yield from root fiber (FiberError)
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-synchrony-1.0.0/lib/em-synchrony.rb:58:in `sync'
    from /tmp/reddy.rb:16:in `method_missing'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/client.rb:119:in `select'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/client.rb:38:in `block in connect'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/event_emitter.rb:8:in `call'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/event_emitter.rb:8:in `block in emit'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/event_emitter.rb:8:in `each'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/event_emitter.rb:8:in `emit'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-hiredis-0.1.0/lib/em-hiredis/connection.rb:15:in `connection_completed'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/eventmachine-1.0.0.beta.4/lib/eventmachine.rb:179:in `run_machine'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/eventmachine-1.0.0.beta.4/lib/eventmachine.rb:179:in `run'
    from /home/blt/.rvm/gems/ruby-1.9.3-p0/gems/em-synchrony-1.0.0/lib/em-synchrony.rb:27:in `synchrony'
    from /tmp/reddy.rb:22:in `<main>'

I find this deeply confusing. Why doesn't it work and am I at fault? If so, what can I do differently? Unless I've glossed over something, this is kosher, per the em-synchrony README.

Contributor

lgierth commented Jan 5, 2012

It looks like EM-Synchrony's patches for EM::Hiredis are not enough. EM::Hiredis::Connection#connection_completed, #receive_data and #unbind might also need to be wrapped in Fibers, similar to the Hiredis::Client methods.

blt commented Jan 5, 2012

I'll look into that, thanks. Can you explain how you arrived at that hypothesis, especially given the exception provided?

Contributor

lgierth commented Jan 5, 2012

The EventMachine reactor is not aware of Fibers and thus simply executes the callbacks (#connection_completed in your case) out of any Fiber context (== inside the "root" Fiber). Fiber.yield will pause the current Fiber and resume the one that created it (its parent). The root Fiber doesn't have a parent and thus a FiberError is raised :)

blt commented Jan 6, 2012

Hmm, okay; I'm still not making the appropriate connection. That is, while I probably have all the pieces needed to move on from here I don't see how to put them together.

This is synchrony's #sync, at least the 1.0.0 version:

    def self.sync(df)
      f = Fiber.current
      xback = proc {|r|
        if f == Fiber.current
          return r
    else
          f.resume r
        end
      } 
      df.callback &xback
      df.errback &xback

      Fiber.yield
    end

Now, if I'm understanding the interaction with eventmachine correctly, #sync:

  • Determines the current running Fiber and stores this as f in xback.
  • On each callback from the deferred either:
    • a value is returned (if the current Fiber and the 'active' fiber are the same)
    • or cooperatively switches to the newly active fiber

Is that right? Okay, assuming that it is, the code above fails because no fibers are created, other than the root--not by me, not by Hiredis and not by EM::Synchrony.sync. Where's the responsibility for this? You've said that

It looks like EM-Synchrony's patches for EM::Hiredis are not enough. EM::Hiredis::Connection#connection_completed, #receive_data and #unbind might also need to be wrapped in Fibers, similar to the Hiredis::Client methods.

but the current monkey-patches only make use of EM::Synchrony.sync I guess that's part of the problem, right? Looking at some of the other monkey-patches--em-mongo and amqp--I notice that mongo does not create its own fibers but amqp does. I imagine this means that em-mongo does create its own fibers and amqp does not? (Also, so far as I can tell, none of the other methods you've mentioned return any sort of deferrable object to feed to #sync.)

I'm afraid I don't yet understand how to fix-up em-hiredis and move on, but I do understand a fair bit more of em-synchrony's internals, so thanks. Think you could offer some more guidance on the em-hiredis problem, too?

Owner

igrigorik commented Jan 7, 2012

scratches head

Guys, it appears to work just fine on this end?

igrigorik { /git/em-synchrony } > /Users/igrigorik/.rvm/rubies/ruby-1.9.3-p0/bin/ruby -S rspec ./spec/hiredis_spec.rb 
...

Finished in 0.63789 seconds
3 examples, 0 failures

Spec @ https://github.com/igrigorik/em-synchrony/blob/master/lib/em-synchrony/em-hiredis.rb

blt commented Jan 7, 2012

Hmm, this fails:

EventMachine.synchrony do
  redis = EM::Hiredis.connect

  redis.set('foo', 'bar')
  puts redis.get('foo')

  EM.stop
end

and this succeeds:

EventMachine.synchrony do
  redis = EM::Hiredis::Client.connect

  redis.set('foo', 'bar')
  puts redis.get('foo')

  EM.stop
end

The em-hiredis README specifically uses the first form when creating a connection, where the em-synchrony specs use the second.


I don't have all the gems needed--like mysql2--on my current machine to run the specs; the above code is taken from my previous comments and should be seen as in that context.

Owner

igrigorik commented Jan 7, 2012

Bah. That's all I have to say about that.. :)

      redis = EM::Hiredis.connect 'redis://127.0.0.1:6379'
      redis.select(0)

That works just fine. The "gotcha" is that the Hiredis.connect interface uses URI to create the connection string, which automatically adds "0" database to the connection string, which in turn fires the select call in the connect callback. Doing the equivalent but manually works just fine (above).

But this highlights a gotcha, namely that if you pass a non-default database name, it'll blow up in the original case as well. So, either we mandate that you have to set the db explicitly later, or have to make a much more extensive shim for em-hiredis.

/cc @mloughran

blt commented Jan 7, 2012

Having an API under synchrony that doesn't follow the conventions of the original API is less than ideal. EM::Hiredis::Client#select is this:

def select(db, &blk)
  @db = db
  method_missing(:select, db, &blk)
end

That's Client#method_missing, which is monkey-patched to invoke EM::Synchrony.sync. I'll get setup later today to run the em-synchrony specs in full, but this works:

require 'em-synchrony' ## v1.0.0                                              
require 'em-hiredis'   ## v0.1.0                                              

module EventMachine::Hiredis
  class Client
    def self.connect(host = 'localhost', port = 6379)
      conn = new(host, port)
      EM::Synchrony.sync conn.connect
      conn
    end

    def select(db, &blk)
      @db = db
      Fiber.new {
        method_missing(:select, db, &blk)
      }.resume
    end

    alias :old_method_missing :method_missing
    def method_missing(sym, *args)
      EM::Synchrony.sync old_method_missing(sym, *args)
    end
  end
end

EventMachine.synchrony do
  redis = EM::Hiredis.connect

  redis.set('foo', 'bar')
  puts redis.get('foo')

  EM.stop
end

The only changes are an added Fiber in Client#setup; I'm using the Hiredis.connect invocation.

igrigorik closed this in 4755c48 Jan 7, 2012

Owner

igrigorik commented Jan 7, 2012

Build from master, should be a-ok now.

blt commented Jan 7, 2012

Using the code from master, this:

EventMachine.synchrony do
  redis = EM::Hiredis.connect
  subscriber = EM::Hiredis.connect

  redis.set('foo', 'bar')
  puts redis.get('foo')

  subscriber.subscribe('bar')

  subscriber.on(:message) do |channel, message|
    p [:message, channel, message]
  end

  EM.add_periodic_timer(1) do
    redis.publish('bar', 'hello')
  end
end

results in a FiberError after one second. However, if I remove the addition of the periodic timer and use an external process to publish the program works as desired.

Contributor

lgierth commented Jan 8, 2012

You need to wrap the timer block in a fiber, you can use EM::Synchrony for
this.add_periodic_timer :)

Am Sonntag, 8. Januar 2012 schrieb Brian L. Troutwine <
reply@reply.github.com

:
Using the code from master, this:

EventMachine.synchrony do
 redis = EM::Hiredis.connect
 subscriber = EM::Hiredis.connect

 redis.set('foo', 'bar')
 puts redis.get('foo')

 subscriber.subscribe('bar')

 subscriber.on(:message) do |channel, message|
   p [:message, channel, message]
 end

 EM.add_periodic_timer(1) do
   redis.publish('bar', 'hello')
 end
end

results in a FiberError after one second. However, if I remove the
addition of the periodic timer and use an external process to publish the
program works as desired.


Reply to this email directly or view it on GitHub:
#103 (comment)

Contributor

lgierth commented Jan 8, 2012

Sorry, I meant EM::Synchrony.add_periodic_timer, my phone keyboard screwed it up.

blt commented Jan 8, 2012

Oh, duh, of course. Thanks very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment