alloy / lowdown

A Ruby client for the HTTP/2 version of the Apple Push Notification Service.
MIT License
120 stars 15 forks source link

actor broken for IO is already registered with selector (on sidekiq job) #13

Closed rayway30419 closed 8 years ago

rayway30419 commented 8 years ago

I create a class for sending push notificatino by lowdown client, and I want to send to production and development device by a recursive call.

If I call this for single sidekiq job, it works well. But when I create multi-job on sidekiq, it actor broken.

the error message is

ERROR -- : Actor crashed! ArgumentError: this IO is already registered with selector /Users/RayWay/.rvm/gems/ruby-2.2.1/gems/celluloid-io-0.17.3/lib/celluloid/io/reactor.rb:42:in `register'

and this is my code (simplified)

def initialize(cert_dev, cert_pro)
    @cert_dev = cert_dev
    @cert_pro = cert_pro
    @pro_mutex = Mutex.new
    @dev_mutex = Mutex.new
  end

 def deliver
     client = dev for loop 1
     client = pro for loop 2
     #connect and send push notification
     deliver() # recursive call
 end

  def production
    client = nil
    @pro_mutex.synchronize do
      require 'lowdown'
      @client ||= Lowdown::Client.production(true, certificate: @cert_pro, keep_alive: true)
      client = @client
    end
    client
  end

  def development
    client = nil
    @dev_mutex.synchronize do
      require 'lowdown'
      @client ||= Lowdown::Client.production(false, certificate: @cert_dev, keep_alive: true)
      client = @client
    end
    client
  end
rayway30419 commented 8 years ago

I try to remove mutex for synchronous and not using multi-threads then it works well, but I have no idea for what happend......

alloy commented 8 years ago

I’m unsure about the specific error message, but I would first look at that instance variable, which you are calling @client in both cases.

rayway30419 commented 8 years ago

In the deliver method, I assign different client for each loop. def deliver_to_users(users, content, arguments, message_id, is_dev = false) Does it have some problem? Or...In the first loop, production and development client instance will be created?!

client = production if is_dev == false
client = development if is_dev == true
alloy commented 8 years ago

My comment is about the @client ||= part, which will initialise only once. E.g.

$ irb
irb(main):001:0> class X
irb(main):002:1>   def foo
irb(main):003:2>     @client ||= :foo
irb(main):004:2>   end
irb(main):005:1> 
irb(main):006:1*   def bar
irb(main):007:2>     @client ||= :bar
irb(main):008:2>   end
irb(main):009:1> end; nil
=> nil
irb(main):010:0> x = X.new
=> #<X:0x007fb42121d770>
irb(main):011:0> x.foo
=> :foo
irb(main):012:0> x.bar
=> :foo
irb(main):013:0> x = X.new
=> #<X:0x007fb42120d398>
irb(main):014:0> x.bar
=> :bar
irb(main):015:0> x.foo
=> :bar
alloy commented 8 years ago

So rename it like so:

def production
    client = nil
    @pro_mutex.synchronize do
      require 'lowdown'
      @pro_client ||= Lowdown::Client.production(true, certificate: @cert_pro, keep_alive: true)
      client = @pro_client
    end
    client
  end

  def development
    client = nil
    @dev_mutex.synchronize do
      require 'lowdown'
      @dev_client ||= Lowdown::Client.production(false, certificate: @cert_dev, keep_alive: true)
      client = @dev_client
    end
    client
  end
rayway30419 commented 8 years ago

It works! Thx for your reply. It's a awesome client for sending push notification!

alloy commented 8 years ago

Ace, glad to hear it works for you and thanks!

alloy commented 8 years ago

@rayway30419 I’m confused, did you leave a comment and deleted that again? (I got an email notification.)

rayway30419 commented 8 years ago

Yes, my sending job works, but I meets some problem. My sidekiq process fork it self and never be terminated after sending notification. I think this problem is caused by sidekiq, not your work...so I delete my comment. Orz

alloy commented 8 years ago

Ok 👍 Please write a comment once you have figured it out, as it might help others in the future.

rayway30419 commented 8 years ago

I find the problem occurs on the recursive call to sending message with development mode. If It execute the recursive call of the method, it still send message with development client, but when all of sending jobs were done, this process will not be terminated and still holds a rails AR connection with database. If I disable the method call of deliver(failure_tokens, true) if is_dev == false, the job terminated well, and I have no idea about this situation now. Do you have any suggestions for this problem? This is my send notification method.

def deliver(tokens, is_dev=false)
  failure_tokens = []
  client = production if is_dev == false
  client = development if is_dev == true
  client.connect do |group|
    tokens.each do |token|
      payload = format_payload #some parameter
      notification = notification(token, payload)
      group.send_notification(notification) do |response|
        if !response.success? 
          failure_tokens.push token if is_dev == false
        end
      end
    end  
  end 
  deliver(failure_tokens, true) if is_dev == false
end
rayway30419 commented 8 years ago

My application uses sidekiq-4.0.2 and celluloid-0.17.3, I found the sidekiq process continuously forks many threads and seems stuck in the celluloid.

2016-04-15T04:02:30.445Z 9484 TID-9o57k WARN: Thread TID-os1g348ro 
2016-04-15T04:02:30.445Z 9484 TID-9o57k WARN: /home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-io-0.17.3/lib/celluloid/io/reactor.rb:58:in `select'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-io-0.17.3/lib/celluloid/io/reactor.rb:58:in `run_once'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/mailbox/evented.rb:50:in `check'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/actor.rb:155:in `block in run'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/timers-4.1.1/lib/timers/group.rb:66:in `wait'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/actor.rb:152:in `run'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/actor.rb:131:in `block in start'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-essentials-0.20.5/lib/celluloid/internals/thread_handle.rb:14:in `block in initialize'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/actor/system.rb:78:in `block in get_thread'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/group/spawner.rb:50:in `call'
/home/deployer/ruby_servers/pushnotificationmanager/shared/bundle/ruby/2.2.0/gems/celluloid-0.17.3/lib/celluloid/group/spawner.rb:50:in `block in instantiate'