[1.6.7-HEAD] ConcurrencyError in safe code #141

Closed
arturaz opened this Issue Apr 23, 2012 · 10 comments

Projects

None yet

2 participants

@arturaz
arturaz commented Apr 23, 2012

Problem

I'm getting ConcurrencyError in monitor synchronized code (inside ActiveRecord).

Code

      # Check-in a database connection back into the pool, indicating that you
      # no longer need this connection.
      #
      # +conn+: an AbstractAdapter object, which was obtained by earlier by
      # calling +checkout+ on this pool.
      def checkin(conn)
        synchronize do
          conn.run_callbacks :checkin do
            conn.expire
            @queue.signal
          end

          release conn
        end
      end

      private

      # This method is only called from #checkin, which is synchronized by +MonitorMixin+.
      def release(conn)
        thread_id = nil

        if @reserved_connections[current_connection_id] == conn
          thread_id = current_connection_id
        else
          thread_id = @reserved_connections.keys.find { |k| # Error in this line
            @reserved_connections[k] == conn
          }
        end

        @reserved_connections.delete thread_id if thread_id
      end

Error

Server launched with: --1.9 -J-Dname=nebula_server -J-Djruby.jit.max=25000 --server -J-XX:+TieredCompilation -J-XX:-UseLoopPredicate -Xbacktrace.style=raw -X+C -J-XX:MaxPermSize=256m

Server has encountered an error!

[2012-04-22 20:44:50.981|worker-world_2|main|error] Threading::Worker crashed!
ConcurrencyError: Detected invalid hash contents due to unsynchronized modifications with concurrent users
java/lang/Thread.java:1567:in `getStackTrace'
org/jruby/runtime/backtrace/TraceType.java:59:in `getBacktraceData'
org/jruby/runtime/backtrace/TraceType.java:111:in `getBacktraceData'
org/jruby/runtime/backtrace/TraceType.java:25:in `getBacktrace'
org/jruby/RubyException.java:160:in `prepareBacktrace'
org/jruby/exceptions/RaiseException.java:205:in `preRaise'
org/jruby/exceptions/RaiseException.java:195:in `preRaise'
org/jruby/exceptions/RaiseException.java:112:in `<init>'
org/jruby/Ruby.java:3381:in `newRaiseException'
org/jruby/Ruby.java:3206:in `newConcurrencyError'
org/jruby/RubyHash.java:1913:in `concurrentModification'
org/jruby/RubyHash.java:1356:in `keys'
org/jruby/RubyHash$i$0$0$keys.gen:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:133:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/gems/activerecord-3.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:294:in `release'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__21$RUBY$release:65535:in `call'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__21$RUBY$release:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:167:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/gems/activerecord-3.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:282:in `checkin'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$block_19$RUBY$checkin:65535:in `call'
org/jruby/runtime/CompiledBlock19.java:121:in `yieldSpecificInternal'
org/jruby/runtime/CompiledBlock19.java:96:in `yieldSpecific'
org/jruby/runtime/Block.java:99:in `yieldSpecific'
/usr/local/rvm/rubies/jruby-head-n16/lib/ruby/1.9/monitor.rb:201:in `__ensure__'
/usr/local/rvm/rubies/jruby-head-n16/lib/ruby/1.9/monitor.rb:200:in `mon_synchronize'
usr$local$rvm$rubies$jruby_minus_head_minus_n16$lib$ruby$$1_dot_9$monitor$method__14$RUBY$mon_synchronize:65535:in `call'
org/jruby/internal/runtime/methods/AliasMethod.java:81:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:142:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:153:in `callIter'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/gems/activerecord-3.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:276:in `checkin'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__20$RUBY$checkin:65535:in `call'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__20$RUBY$checkin:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:167:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/gems/activerecord-3.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:110:in `release_connection'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__7$RUBY$release_connection:65535:in `call'
org/jruby/internal/runtime/methods/DynamicMethod.java:211:in `call'
org/jruby/internal/runtime/methods/CompiledMethod.java:260:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:167:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/gems/activerecord-3.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:121:in `__ensure__'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$gems$activerecord_minus_3_dot_2_dot_3$lib$active_record$connection_adapters$abstract$connection_pool$method__8$RUBY$with_connection:65535:in `call'
org/jruby/internal/runtime/methods/AliasMethod.java:81:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:142:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:153:in `callIter'
/home/spacegame/nebula-server/20120422201639/lib/server/monkey_squad.rb:42:in `with_connection'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$monkey_squad$method__9$RUBY$with_connection:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:142:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:153:in `callIter'
/home/spacegame/nebula-server/20120422201639/lib/server/threading/director/task.rb:31:in `run'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$threading$director$task$method__5$RUBY$run:65535:in `call'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$threading$director$task$method__5$RUBY$run:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:167:in `call'
/home/spacegame/nebula-server/20120422201639/lib/server/threading/worker.rb:21:in `work'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$threading$worker$block_1$RUBY$work:65535:in `call'
org/jruby/runtime/CompiledBlock19.java:121:in `yieldSpecificInternal'
org/jruby/runtime/CompiledBlock19.java:96:in `yieldSpecific'
org/jruby/runtime/Block.java:99:in `yieldSpecific'
/home/spacegame/nebula-server/20120422201639/lib/server/logging/logger.rb:57:in `block'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$logging$logger$method__2$RUBY$block:65535:in `call'
org/jruby/internal/runtime/methods/DynamicMethod.java:219:in `call'
org/jruby/RubyClass.java:611:in `finvoke'
org/jruby/RubyBasicObject.java:1710:in `send19'
org/jruby/RubyKernel.java:2108:in `send19'
org/jruby/RubyKernel$s$send19.gen:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:244:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:250:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:114:in `callVarargs'
/home/spacegame/nebula-server/20120422201639/lib/server/logging/thread_router.rb:7:in `method_missing'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$logging$thread_router$method__1$RUBY$method_missing:65535:in `call'
org/jruby/javasupport/util/RuntimeHelpers.java:497:in `call'
org/jruby/internal/runtime/methods/DynamicMethod.java:219:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:403:in `callMethodMissing'
org/jruby/runtime/callsite/CachingCallSite.java:339:in `cacheAndCall'
org/jruby/runtime/callsite/CachingCallSite.java:212:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:221:in `callIter'
/home/spacegame/nebula-server/20120422201639/lib/server/threading/worker.rb:20:in `work'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$threading$worker$block_0$RUBY$work:65535:in `call'
org/jruby/runtime/CompiledBlock19.java:121:in `yieldSpecificInternal'
org/jruby/runtime/CompiledBlock19.java:96:in `yieldSpecific'
org/jruby/runtime/Block.java:99:in `yieldSpecific'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid/actor.rb:122:in `__ensure__'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$actor$method__18$RUBY$exclusive:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:142:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:148:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid.rb:270:in `exclusive'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$method__41$RUBY$exclusive:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:142:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:153:in `callIter'
/home/spacegame/nebula-server/20120422201639/lib/server/threading/worker.rb:19:in `work'
home$spacegame$nebula_minus_server$$20120422201639$lib$server$threading$worker$method__3$RUBY$work:65535:in `call'
org/jruby/RubyClass.java:592:in `finvoke'
org/jruby/RubyBasicObject.java:1704:in `send19'
org/jruby/RubyKernel.java:2104:in `send19'
org/jruby/RubyKernel$s$send19.gen:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:342:in `cacheAndCall'
org/jruby/runtime/callsite/CachingCallSite.java:212:in `callBlock'
org/jruby/runtime/callsite/CachingCallSite.java:216:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:113:in `callVarargs'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid/calls.rb:98:in `dispatch'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$calls$method__16$RUBY$dispatch:65535:in `call'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$calls$method__16$RUBY$dispatch:65535:in `call'
org/jruby/runtime/callsite/CachingCallSite.java:167:in `call'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid/actor.rb:223:in `handle_message'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$actor$block_9$RUBY$handle_message:65535:in `call'
org/jruby/runtime/CompiledBlock19.java:121:in `yieldSpecificInternal'
org/jruby/runtime/CompiledBlock19.java:96:in `yieldSpecific'
org/jruby/runtime/Block.java:99:in `yieldSpecific'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid/task.rb:45:in `initialize'
/home/spacegame/nebula-server/20120422201639/vendor/bundle/jruby/1.9/bundler/gems/celluloid-53b31cb209dd/lib/celluloid/task.rb:44:in `initialize'
home$spacegame$nebula_minus_server$$20120422201639$vendor$bundle$jruby$$1_dot_9$bundler$gems$celluloid_minus_53b31cb209dd$lib$celluloid$task$block_0$RUBY$initialize:65535:in `call'
org/jruby/runtime/CompiledBlock19.java:163:in `yield'
org/jruby/runtime/CompiledBlock19.java:149:in `yield'
org/jruby/runtime/Block.java:146:in `yieldArray'
org/jruby/ext/fiber/ThreadFiber.java:38:in `run'
java/util/concurrent/ThreadPoolExecutor.java:1110:in `runWorker'
java/util/concurrent/ThreadPoolExecutor.java:603:in `run'
java/lang/Thread.java:722:in `run'
@headius headius was assigned Apr 26, 2012
@headius
Member
headius commented Apr 26, 2012

Ok, so here's the deal. I see no synchronization of the reserved connections hash in the release method. That synchronization might be happening somewhere else, but if it's not this could certainly be a problem in AR.

Non-thread-safe resources need to be synchronized on read and write, since reading from a resource while it is being written can cause exactly the same problems as two writes at the same time.

You could try as a workaround the following code, which must run after the the specified resource is available (some time late in boot):

require 'jruby'

that_connection_pool.extend JRuby::Synchronized

This will wrap all methods with synchronization...which is a blunt tool, but it might prove there's some sync missing in AR itself.

@arturaz
arturaz commented Apr 26, 2012

Did you see the comment?

This method is only called from #checkin, which is synchronized by +MonitorMixin+

@headius
Member
headius commented Apr 26, 2012

Ok, I missed that. However my point does still stand...the monitor should be guaranteeing only one thread is active at a time, which should prevent the concurrency error. But it does give me pause...we have cases where an internal exception was improperly masked as a concurrency error. I'll look into the code a bit more.

@headius
Member
headius commented Apr 26, 2012

Ok, so far I see no clear indication that this would be a JRuby bug. I'd recommend trying the JRuby::Synchronized mechanism and opening a bug with Rails. I'll support you there.

@arturaz
arturaz commented Apr 26, 2012

Could it be that MonitorMixin is flawed on jruby?

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Charles Oliver Nutter reply@reply.github.com wrote:

Ok, so far I see no clear indication that this would be a JRuby bug. I'd recommend trying the JRuby::Synchronized mechanism and opening a bug with Rails. I'll support you there.


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

@headius
Member
headius commented Apr 29, 2012

Anything's possible, but that seems unlikely.

The synchronize method here is mon_synchronize from MonitorMixin. mon_synchronize acquires the lock (a mutex in @mon_mutex in mon_enter, and releases it in mon_exit. The code within that block should be guaranteed to only run in one thread at a time, and I see no obvious bugs in the library.

I also reviewed commit logs for monitor.rb. Within the past 4 years, almost no changes have been made other than formatting and style changes. The most recent substantive change was in 2007, but we're using files much newer than that and even then the changes were ok.

Mutex acquisition and release in JRuby is implemented in the Mutex class, which uses either Java synchronization (object monitors) or a ReentrantLock. The implementation is quite simple, and I would not expect there to be bugs.

So...it's certainly possible there's something I've missed, but I still tend to believe there's an issue in Rails where they're not controlling access to this cache everywhere they should be.

@arturaz
arturaz commented May 5, 2012

NameError: uninitialized constant JRuby::Synchronized

On 1.6.8-dev

@arturaz
arturaz commented May 9, 2012

Tossed this to Rails guys, no response so far...

@headius
Member
headius commented May 9, 2012

Oops, I botched my example...you need to require 'jruby/synchronized' for JRuby::Synchronized.

@arturaz
arturaz commented May 26, 2012

Ok, it seems that this is rails bug afterall...

      def connection
        @reserved_connections[current_connection_id] ||= checkout
      end
      def release_connection(with_id = current_connection_id)
        conn = @reserved_connections.delete(with_id)
        checkin conn if conn
      end

No sync!

@arturaz arturaz closed this May 26, 2012
@eregon eregon added a commit that referenced this issue Oct 28, 2015
@eregon eregon Squashed 'spec/ruby/' changes from fa9e1cd..92311a8
92311a8 Module#prepend is private in ruby 2.0
e3283d5 Module#include and #prepend should be defined directly on Module
1afea36 Specify TCPServer#accept and #accept_nonblock when closed
c4bb84f Add spec for String#force_encoding called on a frozen string.
0f1e1db Add spec for alias and undef raising a NameError which is not a NoMethodError
86e10f3 New spec for Kernel#exit across threads
dfb2aa0 Refactor a couple Module#prepend specs
a6bd065 Add a spec for Range#dup
8aa7b1e Super from a class method must work with prepended modules.
591ff4f Add a few more specs for defined? with scoped constants.
b99f44c Add a few spec about the return value with ensure
ee87540 Add yield-to-lambda cases that were not all consistent in JRuby.
156bb2b proc.call on object that calls #to_ary which returns nil should return original object
9145998 Add spec for alias on top level
a924c74 Add specs for results of begin-end blocks
193cf30 New spec for setting visibility from a block
f987230 Clarify private spec
22e0298 Add spec for nested methods in a "def expr.meth"
142a876 Add spec for nested methods in instance_eval
8d7b807 Ensure the variable can be read from Binding#eval after Binding#local_variable_set
fe35f8a Actually use a shadowed variable in Binding#local_variables spec
092639a Remove inappropriate spec
842d3aa Fix UnboundMethod#super_method to not be order dependent
bfd0a7e Merge pull request #155 from wied03/master
f6ebc9b Follow up for ruby/spec#153
5b76fa0 Merge pull request #154 from wied03/master
948839f Test struct methods ending in ?, !
adbb7de Exact match for without_test_modules
58f9bad Fix style in Module#ancestors spec
da511ce Merge pull request #153 from wied03/tighten_ancestors
01ba680 Exclude more test modules
278546d Tighten up ancestors specs
c49e802 Add a few more check that methods do not end up defined on Object
a81b5d3 Merge pull request #150 from wied03/instance_eval_class_mod
e27298c Merge pull request #148 from wied03/module_instance_methods
35164b4 Test instance eval/exec on classes and modules
7a45ecb Ensure including a module doesn't prevent instance_methods from working
b188d8e Merge pull request #147 from wied03/multiple_remove_meth
f554ff9 Clarify the file organization
004151e Specify that Class#new yield the new Class
dba4b94 Merge pull request #149 from wied03/mod_class_to_s
98235ef A start at module/class#to_s
4d1cf90 Merge pull request #152 from kachick/struct-new-passes-subclass-to-block
8fdfc4f Add a spec for block parameter in Struct.new
f390fcb Improve definition spaces for some Struct.new behaviors
7df9151 Merge pull request #151 from wied03/struct_with_block
4890c67 Improve struct with block tests
9ff4c13 Fully test multiple remove_methods at once
122e1e1 Merge pull request #145 from wied03/master
6b57785 Another case that wasn't working on Opal
4025c0e Only allow #extend to throw the error in Kernel#extend
7551ce6 Kernel#extend raises a TypeError when the argument is not a Module
df5dfc6 Merge pull request #143 from ruby/elia/unnecessary-mutable-string-use
a7acf82 Object instead of a String for singleton spec
c9cc7ae OpenStruct#new_ostruct_member is now a protected method
1b42fb9 Do not access private internal state in the OpenStruct#new_ostruct_member spec
c7f0b73 remove ruby_bug related to old releases
5786f3a remove duplicate specs Float#<=>.
f5323b8 remove trailing spaces
1508e8b Merge pull request #142 from ruby/elia/time_yday
2463dda Simplify the Time#yday
7472ad9 Test Time#yday for each day of each month
c9ff9cf Merge pull request #141 from ruby/elia/not_match_spec
aa56fe8 Minor fixes
17c66fa Add initial specs for Kernel#!~
391d026 Merge pull request #139 from wied03/master
af22315 Merge pull request #140 from wied03/defined_improve
db9a977 Test more granular defined? scenarios
49012ea Added another exception inspect case that wasn't covered
7512517 Update to latest ruby releases
d50b1bd Fix specdoc in Regexp#options
23585e7 Merge pull request #137 from wied03/master
fcf6a05a Cover another regexp case that was failing in Opal
bde1725 Opal is treating // as if it was created with Regexp.allocate, so add a non encoding based options test of // that it can execute and also test that match fails the same way options does
7819cb3 FreeBSD and NetBSD supports birthtime

git-subtree-dir: spec/ruby
git-subtree-split: 92311a8114663b4ae81a086699c4510271fff1ab
4a0ae9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment