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

recover from (concurrent) handler removal #6

Merged
merged 7 commits into from
Nov 18, 2016

Conversation

kares
Copy link
Contributor

@kares kares commented Oct 25, 2016

... a follow up from #5

(under JRuby) for some specific use-cases more concurrency fixes are needed to run AHN safely.

there's 2 errors @cloudvox managed to run into (using JRuby 1.7) which both related to the same issue

  1. reported (by others) and explained at Punchblock::Translator::Asterisk crashes with NoMethodError punchblock#234 (comment)
  2. triggering a (tmp) handler while another trigger_handler :ami is happening concurrently, on JRuby :
ConcurrencyError: Detected invalid array contents due to unsynchronized modifications with concurrent users
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:180:in `handlers_of_type'
    org/jruby/RubyArray.java:1617:in `each'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:178:in `handlers_of_type'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:131:in `trigger_handler'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/punchblock-42cbe4572035/lib/punchblock/translator/asterisk/call.rb:183:in `process_ami_event'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/punchblock-42cbe4572035/lib/punchblock/translator/asterisk.rb:230:in `ami_dispatch_to_or_create_call'
    org/jruby/RubyHash.java:1357:in `each_pair'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/punchblock-42cbe4572035/lib/punchblock/translator/asterisk.rb:228:in `ami_dispatch_to_or_create_call'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/punchblock-42cbe4572035/lib/punchblock/translator/asterisk.rb:112:in `handle_ami_event'
    org/jruby/RubyKernel.java:1932:in `public_send'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/calls.rb:25:in `dispatch'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/calls.rb:122:in `dispatch'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/actor.rb:322:in `handle_message'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/actor.rb:416:in `task'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/tasks.rb:55:in `initialize'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/tasks.rb:47:in `initialize'
    /srv/phone/apptastic/shared/bundle/jruby/1.9/bundler/gems/celluloid-7af615463b10/lib/celluloid/tasks/task_fiber.rb:13:in `create'

they both happen since PB::Translator::Asterisk::Call instances are no longer actors ... the changes proposed assure this case is covered correctly.

the later can be reproduced by running the following minimal (only HGH) script under JRuby ... simply TIMES=1000 concurrency-issue-simplified2.rb should do (make sure to gem install concurrent-ruby)

alternatives to solving the issue would be :

  1. introduce mutex locking (possibly only around Call's handler triggering)
  2. rewrite HGH to be fully thread-safe (there's still no guarantee a tmp handler won't fire twice)
  3. avoid tmp handler registration in Call instances (there's also one in Component::Record)

@sfgeorge
Copy link
Member

@benlangfeld could you share your thoughts when you get a chance?

Copy link
Member

@benlangfeld benlangfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog entry please :)

@benlangfeld
Copy link
Member

This just needs conflicts resolving :)

@kares
Copy link
Contributor Author

kares commented Nov 18, 2016

meh the changelog ... on it!

…ss garbage)

... mostly future proof as with current TS::Cache setup it is not expected to happen
tmp handlers are removed after they're called from the Array holding them
in rare cases when the removal happens just as handlers_of_type (for the same type) are being push-ed among collected `values`, the value Array to be added might get modified concurrently - under JRuby : 

```
#<ConcurrencyError: Detected invalid array contents due to unsynchronized modifications with concurrent users>
  /opt/local/rvm/gems/jruby-1.7.4@dialogtech/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:180:in `handlers_of_type'
  org/jruby/RubyArray.java:1617:in `each'
  /opt/local/rvm/gems/jruby-1.7.4@dialogtech/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:178:in `handlers_of_type'
  /opt/local/rvm/gems/jruby-1.7.4@dialogtech/bundler/gems/has-guarded-handlers-4d4980673764/lib/has_guarded_handlers.rb:131:in `trigger_handler'
```

in such cases re-trying the operation (re-reading the value Array) is safe and unlikely to fail again!
the ConcurrencyError on JRuby's Array impl is mostly a nice feature
... it's hard to reproduce concurrent behavior - but its smt at least!
`class ConcurrencyError < ThreadError` not available on MRI
@kares
Copy link
Contributor Author

kares commented Nov 18, 2016

rebased, thanks for looking into it Ben.

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

Successfully merging this pull request may close these issues.

3 participants