search

celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Strange handling of IOError #151

Closed marshall-lee closed 8 years ago

marshall-lee commented 8 years ago

Not sure if it's a right repo for reporting this but here it is.

I'm trying to use celluloid-redis inside of Celluloid::IO actor.

require 'celluloid/current'
require 'celluloid/io'
require 'redis'

class RedisPusher
  include Celluloid::IO

  def push
    redis.incr 'var'
  rescue
    puts 'oh no!'
    p $!
    puts $!.backtrace
    raise
  end

  def redis
    @redis ||= Redis.new(driver: :celluloid)
  end
end

class Worker
  include Celluloid

  def initialize
    async.work
  end

  def work
    sleep 1
    loop do
      30.times do
        Celluloid::Actor[:pusher].async.push
      end
      sleep 0.5
    end
  rescue
    p $!
    retry
  end
end

RedisPusher.supervise as: :pusher
Worker.supervise as: :worker

sleep

And after some amout of time i'm getting this error:

oh no!
#<IOError: closed stream>
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.0/lib/celluloid/io/stream.rb:41:in `block in sysread'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.0/lib/celluloid/io/stream.rb:389:in `synchronize'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.0/lib/celluloid/io/stream.rb:39:in `sysread'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.0/lib/celluloid/io/stream.rb:322:in `fill_rbuff'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.0/lib/celluloid/io/stream.rb:159:in `gets'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-redis-0.0.2/lib/redis/connection/celluloid.rb:53:in `read'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:248:in `block in read'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:236:in `io'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:247:in `read'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:112:in `block in call'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:217:in `block (2 levels) in process'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:353:in `ensure_connected'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:207:in `block in process'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:292:in `logging'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:206:in `process'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis/client.rb:112:in `call'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis.rb:618:in `block in incr'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis.rb:37:in `block in synchronize'
/home/vladimir/.rvm/rubies/ruby-2.2.1/lib/ruby/2.2.0/monitor.rb:211:in `mon_synchronize'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis.rb:37:in `synchronize'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/redis-3.2.1/lib/redis.rb:617:in `incr'
asy.rb:10:in `push'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/calls.rb:28:in `public_send'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/calls.rb:28:in `dispatch'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/call/async.rb:7:in `dispatch'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/cell.rb:50:in `block in dispatch'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/cell.rb:76:in `block in task'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/actor.rb:338:in `block in task'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/task.rb:44:in `block in initialize'
/home/vkochnev/.rvm/gems/ruby-2.2.1/gems/celluloid-0.17.1.1/lib/celluloid/task/fibered.rb:14:in `block in create'
#<NoMethodError: undefined method `async' for nil:NilClass>
#<NoMethodError: undefined method `async' for nil:NilClass>
#<NoMethodError: undefined method `async' for nil:NilClass>
#<NoMethodError: undefined method `async' for nil:NilClass>
#<NoMethodError: undefined method `async' for nil:NilClass>
#<NoMethodError: undefined method `async' for nil:NilClass>
...

As you can see Celluloid::Actor[:pusher] does not restart!

marshall-lee commented 8 years ago

I'm not getting this error if at least one these conditions met:

And also I noticed that if I change push method this way:

  def push
    redis.incr 'var'
    raise IOError
  rescue
    puts 'oh no!'
    p $!
    puts $!.backtrace
    raise
  end

then it fails instantly on the first call to push but it doesn't restart. And without redis.incr 'var' call it restarts fine as I mentioned above.

marshall-lee commented 8 years ago

Maybe celluloid-ruby is somehow not thread safe but why the supervised actor is not restarted?

Asmod4n commented 8 years ago

Have you tried it with

require 'celluloid/redis'

yet ?

Asmod4n commented 8 years ago

Also,

  def redis
    @redis ||= Redis.new(driver: :celluloid)
  end

doesn't look safe, can you try it to setup the redis object in the initializer?

marshall-lee commented 8 years ago

require 'celluloid/redis' just loads redis-ext that monkeypatches _parse_driver. And also it fails: NoMethodError: undefined method 'new' for Celluloid::Redis:Module.

But this monkeypatch is not required now since _parse_driver from redis-rb calls require dynamically

marshall-lee commented 8 years ago

Okay, I changed my example to this:

require 'celluloid/current'
require 'celluloid/io'
require 'celluloid/redis'

class RedisPusher
  include Celluloid::IO

  def initialize
    @redis = ::Redis.new(driver: :celluloid)
  end

  attr_reader :redis

  def push
    redis.incr 'var'
  rescue
    puts 'oh no!'
    p $!
    puts $!.backtrace
    raise
  end
end

class Worker
  include Celluloid

  def initialize
    async.work
  end

  def work
    sleep 1
    loop do
      30.times do
        Celluloid::Actor[:pusher].async.push
      end
      sleep 0.5
    end
  rescue
    p $!
    retry
  end
end

RedisPusher.supervise as: :pusher
Worker.supervise as: :worker

sleep

And it still fails.

marshall-lee commented 8 years ago

NoMethodError: undefined method 'new' for Celluloid::Redis:Module

It's just a minor issue in celluloid-redis — it defines Celluloid::Redis module in version.rb, don't pay attention to this for now.

marshall-lee commented 8 years ago

Btw, why @redis ||= Redis.new(driver: :celluloid) is not threadsafe?

marshall-lee commented 8 years ago

Simpler example (without race) that fails and doesn't restart supervisor too:

require 'celluloid/current'
require 'celluloid/io'
require 'celluloid/redis'

class RedisPusher
  include Celluloid::IO

  def initialize
    @redis = ::Redis.new(driver: :celluloid)
  end

  attr_reader :redis

  def push
    redis.incr 'var'
    raise IOError
  rescue
    puts 'oh no!'
    p $!
    puts $!.backtrace
    raise
  end
end

RedisPusher.supervise as: :pusher

Celluloid::Actor[:pusher].async.push
sleep 0.1
Celluloid::Actor[:pusher].async.push

Second call to async.push fails with this error:

undefined method `async' for nil:NilClass (NoMethodError)
marshall-lee commented 8 years ago

Example without celluloid-redis at all:

require 'celluloid/current'
require 'celluloid/io'

class Server
  include Celluloid::IO
  finalizer :shutdown

  def initialize
    system('rm -f /tmp/something')
    @server = UNIXServer.new('/tmp/something')
    async.run
  end

  def run
    loop { async.handle_connection @server.accept }
  end

  def handle_connection(socket)
    loop { socket.write socket.readpartial(4096) }
  rescue EOFError
    socket.close
  end

  def shutdown
    @server.close if @server
  end
end

class Client
  include Celluloid::IO

  def initialize
    @socket = UNIXSocket.new('/tmp/something')
  end

  def push(str)
    @socket.write str
    puts @socket.read
    raise IOError
  end
end

Server.supervise as: :server
Client.supervise as: :client

Celluloid::Actor[:client].async.push('1')
sleep 0.1
Celluloid::Actor[:client].async.push('2')

Also fails with:

undefined methodasync' for nil:NilClass (NoMethodError)`

Also without @socket.read supervised actor restarts fine.

Asmod4n commented 8 years ago

must have mistaken

  def redis
    @redis ||= Redis.new(driver: :celluloid)
  end

with

  def self.redis
    @redis ||= Redis.new(driver: :celluloid)
  end

The latter is not thread safe.

sorry ^^

Asmod4n commented 8 years ago

Btw, if you want to throw a exception but don't crash a actor you can use fail IOError

marshall-lee commented 8 years ago

@Asmod4n simple explanation is here: https://github.com/celluloid/celluloid/pull/666#issuecomment-134156510

marshall-lee commented 8 years ago

should be fixed by https://github.com/celluloid/celluloid/pull/666