Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EM-Synchrony doesn't work with Enumerator #114

Open
prepor opened this issue Mar 11, 2012 · 8 comments
Open

EM-Synchrony doesn't work with Enumerator #114

prepor opened this issue Mar 11, 2012 · 8 comments

Comments

@prepor
Copy link
Contributor

prepor commented Mar 11, 2012

Hello! Simple case:

require 'em-synchrony'
EM.synchrony do
  start = Time.now
  enum = Enumerator.new do |y|
    3.times do |i|
      EM::Synchrony.sleep 100
      y.yield i
    end
  end
  puts enum.next
  puts "Time: #{Time.now - start}"
  EM.stop
end

And it will return value (nil) instantly, because Enumerator uses Fibers internally and we have yielded from its fiber in EM::Synchrony.sleep.

@igrigorik
Copy link
Owner

Hmmm, wow.. I am really surprised this hasn't show up before! Nice one :-)

This seems to do the trick for me:

require 'em-synchrony'

EM.synchrony do
  start = Time.now

  enum = Enumerator.new do |y|
    3.times do |i|
        y << i
      end
  end

  enum.each do |v|
    puts v
    EM::Synchrony.sleep 1
  end

  puts "Time: #{Time.now - start}"
  EM.stop
end

# $> ruby test.rb 
# 0
# 1
# 2
# Time: 3.001568

@fl00r
Copy link

fl00r commented Mar 12, 2012

The actual problem was with slow IO in Enumerator. The usecase was something like

enumerator = Enumerator.new do |enum|
  ActiveRecordModel.some.query.each do |item|
    enum.yield item.title
  end
end
EM::Synchrony::FiberIterator.new(enumerator, 10).each do |title|
  # processing
end

And in EM::Iterator next is called on enumerators, so it fails

@prepor
Copy link
Contributor Author

prepor commented Mar 12, 2012

In fact, there are more simple trick:

require 'em-synchrony'
EM.synchrony do
  start = Time.now
  enum = Enumerator.new do |y|
    3.times do |i|
      EM::Synchrony.sleep 1
      y.yield i
    end
  end
  enum.each { |i| puts i }
  puts "Time: #{Time.now - start}"
  EM.stop
end

So, rule is sound like this: "do not use em-sync in body of enumerator with #next"

@funny-falcon
Copy link
Contributor

@prepor
Copy link
Contributor Author

prepor commented Mar 12, 2012

Another way:

ROOT = Fiber.current

EM.run do
  Fiber.new do
    en = Enumerator.new do |y|
      3.times do |i|
        f = Fiber.current
        EM.add_timer(1) { f.transfer }
        ROOT.transfer
        y << i
      end
    end
    puts en.next
    EM.stop
  end.resume
end

Fiber.yield in EM.Synchrony is always "ok, we can't do anything more in this fiber and must wait callback", so always transfer root fiber (with event loop) is ok, i think.

@igrigorik
Copy link
Owner

@funny-falcon's patch seems to do the trick.. but I'm having a hard time trying to figure out the actual patch in there :-)

@igrigorik
Copy link
Owner

So, do we want to merge any of this into master? If not, I'll close the bug.

@prepor
Copy link
Contributor Author

prepor commented Jun 21, 2012

But bug are exist, but it is impossible to fix it in current EM-Syncrhony correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants