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

Match data missing in concurrent proc #3031

Open
donv opened this issue Jun 10, 2015 · 27 comments
Open

Match data missing in concurrent proc #3031

donv opened this issue Jun 10, 2015 · 27 comments

Comments

@donv
Copy link
Member

donv commented Jun 10, 2015

Hi!

Here is a simplified example from an actual production app. It fails every time for me on JRuby 1.7.20. Am I doing something wrong?

chars = ("a".."z").to_a

block = proc do |i|
  s = 5.times.map {chars.sample}.join
  if /(a)/ =~ s
    raise "Match is missing (#{i}): #{s}: #{$1.inspect}" unless $1
  end
end

threads = 20.times.map{Thread.start{400.times{|i| block.call i}}}
threads.each(&:join)
@donv
Copy link
Member Author

donv commented Jun 11, 2015

From IRC #jruby:

14:41 <donV> Hi all! Can anybody reproduce? https://github.com/jruby/jruby/issues/3031 
14:46 <bbrowning> donV: RuntimeError: Match is missing (145): gnrak: "a" 
14:46 <ebarrett> RuntimeError: Match is missing (155): sakaw: nil 
14:47 <ebarrett> ^ on a couple of days ago master branch
14:47 <donV> Thanks! 
...
18:46 <enebo> donV: yeah it seems like somehow $~ is getting set and that dynamic scope where it lives is comopeting with another thread 
18:46 <enebo> donV: Should not be doing that so we need to fix it 

@billdueber
Copy link

Making an explicit match object works fine. (Just trying to be helpful while being ignorant)

chars = ("a".."z").to_a

block = proc do |i|
  s = 5.times.map {chars.sample}.join
  m = /(a)/.match(s)
  if m
    raise "Match is missing (#{i}): #{s}: #{$1.inspect}" unless m[1]
  end
end

threads = 20.times.map{Thread.start{400.times{|i| block.call i}}}
threads.each(&:join)

@headius
Copy link
Member

headius commented Jul 7, 2015

There does appear to be a behavioral difference from MRI here:

[] ~/projects/jruby $ rvm ruby-2.2 do ruby -e 'p = proc { p $~; "foo" =~ /foo/ }; Thread.new {p.call}.join; Thread.new{p.call}.join'
nil
nil

[] ~/projects/jruby $ jruby -e 'p = proc { p $~; "foo" =~ /foo/ }; Thread.new {p.call}.join; Thread.new{p.call}.join'
nil
#<MatchData "foo">

So the $~ are both frame-local and thread-local; we may need some logic in our proc/binding to duplicate the frame if it did not original in the same thread.

@headius
Copy link
Member

headius commented Nov 19, 2015

This is likely causing your issue, @reidmorrison.

@reidmorrison
Copy link

@headius thank you for taking the time to meet with me at RubyConf to research the issue.

One other thought, in our case the globally defined procs are called using instance_exec so that they can access the data from a local object:

object.instance_exec(string, &proc)

The object who's scope is being used is specific to each thread and is not shared.

Per your suggestion we are looking into converting our global procs into methods, which will also result in a very nice performance improvement.

@headius
Copy link
Member

headius commented Nov 23, 2015

@reidmorrison I don't think the instance_exec would save you from the backref being shared, since it lives at a higher level in the closure's captured execution state. instance_exec with a Proc is also generally much faster than eval forms, so perf-wise it's not terrible. But yes, I agree with moving to a different model that doesn't use a Proc and doesn't use instance_exec.

@headius
Copy link
Member

headius commented Mar 11, 2016

FYI, I tried a prototype fix that uses threadlocals inside our Frame object to ensure they're really truly thread-local. It's not the "right" fix, and it makes Frame objects much heavier to clone, but it did appear to pass specs fine.

@billdueber
Copy link

billdueber commented May 5, 2016

I can't get a simple example to fail for me, but when I abort with the stacktrace in my production code, I get this:

jruby 9.1.0.0 (2.3.0) 2016-05-02 a633c63 Java HotSpot(TM) 64-Bit Server VM 25.0-b70 on 1.8.0-b132 +jit [darwin-x86_64]
java.lang.ClassCastException: org.jruby.RubyNil cannot be cast to org.jruby.RubyMatchData
    at org.jruby.RubyString.regexSplit19(RubyString.java:3542)
    at org.jruby.RubyString.splitCommon19(RubyString.java:3512)
    at org.jruby.RubyString.split19(RubyString.java:3466)
    at org.jruby.RubyString$INVOKER$i$split19.call(RubyString$INVOKER$i$split19.gen)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:161)
    at indexers.umich.invokeOther2:split(indexers/umich.rb)
    at indexers.umich.RUBY$block$load_config_file$2(indexers/umich.rb:71)
    at org.jruby.runtime.CompiledIRBlockBody.yieldDirect(CompiledIRBlockBody.java:156)
    at org.jruby.runtime.BlockBody.yield(BlockBody.java:110)
    at org.jruby.runtime.Block.yield(Block.java:167)
    at org.jruby.RubyArray.each(RubyArray.java:1593)
    at org.jruby.RubyArray$INVOKER$i$0$0$each.call(RubyArray$INVOKER$i$0$0$each.gen)
    at org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:139)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:145)
    at indexers.umich.invokeOther11:each(indexers/umich.rb)
    at indexers.umich.RUBY$block$load_config_file$0(indexers/umich.rb:70)
    at org.jruby.runtime.CompiledIRBlockBody.callDirect(CompiledIRBlockBody.java:145)
    at org.jruby.runtime.MixedModeIRBlockBody.callDirect(MixedModeIRBlockBody.java:113)
    at org.jruby.runtime.IRBlockBody.call(IRBlockBody.java:64)
    at org.jruby.runtime.Block.call(Block.java:126)
    at org.jruby.RubyProc.call(RubyProc.java:342)
    at org.jruby.RubyProc.call19(RubyProc.java:326)
    at org.jruby.RubyProc$INVOKER$i$0$0$call19.call(RubyProc$INVOKER$i$0$0$call19.gen)
    at org.jruby.internal.runtime.methods.DynamicMethod.call(DynamicMethod.java:217)
    at org.jruby.internal.runtime.methods.DynamicMethod.call(DynamicMethod.java:213)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:225)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.step.invokeOther21:call(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer/step.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.step.RUBY$method$execute$0(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer/step.rb:149)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.invokeExact(CompiledIRMethod.java:245)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.call(CompiledIRMethod.java:110)
    at org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:163)
    at org.jruby.internal.runtime.methods.DynamicMethod.call(DynamicMethod.java:197)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:161)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.invokeOther11:execute(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.RUBY$block$map_to_context!$4(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb:346)
    at org.jruby.runtime.CompiledIRBlockBody.yieldDirect(CompiledIRBlockBody.java:156)
    at org.jruby.runtime.IRBlockBody.yieldSpecific(IRBlockBody.java:73)
    at org.jruby.runtime.Block.yieldSpecific(Block.java:136)
    at org.jruby.ir.runtime.IRRuntimeHelpers.yieldSpecific(IRRuntimeHelpers.java:480)
    at org.jruby.ir.targets.YieldSite.yieldSpecific(YieldSite.java:114)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.RUBY$method$log_mapping_errors$0(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb:400)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.invokeExact(CompiledIRMethod.java:258)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.call(CompiledIRMethod.java:119)
    at org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:198)
    at org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:203)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:209)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.invokeOther15:log_mapping_errors(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.RUBY$block$map_to_context!$3(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb:345)
    at org.jruby.runtime.CompiledIRBlockBody.yieldDirect(CompiledIRBlockBody.java:156)
    at org.jruby.runtime.BlockBody.yield(BlockBody.java:110)
    at org.jruby.runtime.Block.yield(Block.java:167)
    at org.jruby.RubyArray.each(RubyArray.java:1593)
    at org.jruby.RubyArray$INVOKER$i$0$0$each.call(RubyArray$INVOKER$i$0$0$each.gen)
    at org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:139)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:145)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.invokeOther21:each(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.RUBY$method$map_to_context!$0(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb:339)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.invokeExact(CompiledIRMethod.java:245)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.call(CompiledIRMethod.java:110)
    at org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:163)
    at org.jruby.internal.runtime.methods.DynamicMethod.call(DynamicMethod.java:197)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:161)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.invokeOther0:map_to_context!(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.indexer.RUBY$block$process$1(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/indexer.rb:474)
    at org.jruby.runtime.CompiledIRBlockBody.yieldDirect(CompiledIRBlockBody.java:156)
    at org.jruby.runtime.BlockBody.yield(BlockBody.java:118)
    at org.jruby.runtime.Block.yieldArray(Block.java:179)
    at org.jruby.ir.runtime.IRRuntimeHelpers.yield(IRRuntimeHelpers.java:476)
    at org.jruby.ir.targets.YieldSite.yield(YieldSite.java:87)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.traject_minus_2_dot_3_dot_1_minus_java.lib.traject.thread_pool.RUBY$block$maybe_in_thread_pool$1(/Users/dueberb/.gem/jruby/2.3.0/gems/traject-2.3.1-java/lib/traject/thread_pool.rb:110)
    at org.jruby.runtime.CompiledIRBlockBody.callDirect(CompiledIRBlockBody.java:145)
    at org.jruby.runtime.IRBlockBody.call(IRBlockBody.java:64)
    at org.jruby.runtime.Block.call(Block.java:126)
    at org.jruby.RubyProc.call(RubyProc.java:342)
    at org.jruby.RubyProc.call19(RubyProc.java:326)
    at org.jruby.RubyProc$INVOKER$i$0$0$call19.call(RubyProc$INVOKER$i$0$0$call19.gen)
    at org.jruby.internal.runtime.methods.DynamicMethod.call(DynamicMethod.java:171)
    at org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:67)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.concurrent_minus_ruby_minus_1_dot_0_dot_2_minus_java.lib.concurrent.executor.java_executor_service.invokeOther2:call(/Users/dueberb/.gem/jruby/2.3.0/gems/concurrent-ruby-1.0.2-java/lib/concurrent/executor/java_executor_service.rb)
    at Users.dueberb.$_dot_gem.jruby.$2_dot_3_dot_0.gems.concurrent_minus_ruby_minus_1_dot_0_dot_2_minus_java.lib.concurrent.executor.java_executor_service.RUBY$method$run$0(/Users/dueberb/.gem/jruby/2.3.0/gems/concurrent-ruby-1.0.2-java/lib/concurrent/executor/java_executor_service.rb:94)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.invokeExact(CompiledIRMethod.java:219)
    at org.jruby.internal.runtime.methods.CompiledIRMethod.call(CompiledIRMethod.java:92)
    at org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:93)
    at Concurrent$$JavaExecutorService$$Job_1181670822.run(Concurrent$$JavaExecutorService$$Job_1181670822.gen:13)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:744)
org.jruby.RubyNil cannot be cast to org.jruby.RubyMatchData

@kares kares added this to the JRuby 9.1.1.0 milestone May 5, 2016
@headius headius modified the milestones: JRuby 9.1.1.0, JRuby 9.1.2.0 May 11, 2016
@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 23, 2016
@headius
Copy link
Member

headius commented Aug 16, 2016

@billdueber That should probably go in a separate report, if it hasn't been fixed already.

I'm now a little confused about this. MRI does appear to give each thread its own area in some cases, and share an area in others.

$ ruby23 -e 'p = proc { p $~; "foo" =~ /foo/ }; Thread.new {p.call}.join; Thread.new{p.call}.join'
nil
nil

$ ruby23 -e 'def foo; proc { p $~; "foo" =~ /foo/ }; end; p = foo; Thread.new {p.call}.join; Thread.new{p.call}.join'
nil
#<MatchData "foo">

$ ruby23 -e 'p = proc { p $~; "foo" =~ /foo/ }; def foo(p); Thread.new {p.call}.join; Thread.new{p.call}.join; end; foo(p)'
nil
#<MatchData "foo">

$ ruby23 -e 'class Foo; P = proc { p $~; "foo" =~ /foo/ }; def foo; Thread.new {P.call}.join; Thread.new{P.call}.join; end; end; Foo.new.foo'
nil
#<MatchData "foo">

So for this reproduction, MRI shares the frame across threads for all but the pure-toplevel case.

It may be that this bug actually does exist in MRI, but because you're never running Ruby code across threads it doesn't (often?) materialize.

I need some clarification from @nobu and @ko1 here. How does MRI deal with these situations?

@headius
Copy link
Member

headius commented Aug 16, 2016

Another case, which shows that the frame slot for $~ is thread-local only when both the thread body and the captured proc have the same frame (e.g. not just toplevel):

$ ruby23 -e 'def foo; p = proc { p $~; "foo" =~ /foo/ }; Thread.new {p.call}.join; Thread.new{p.call}.join; end; foo'
nil
nil

@headius
Copy link
Member

headius commented Aug 16, 2016

Another case, showing that it doesn't have to be an escaped frame (one whose original activation has gone away)...it can simply be a still-active frame but the threading happens down-stack:

$ ruby23 -e 'def foo; p = proc { p $~; "foo" =~ /foo/ }; bar(p); end; def bar(p); Thread.new {p.call}.join; Thread.new{p.call}.join; end; foo'
nil
#<MatchData "foo">

@headius
Copy link
Member

headius commented Aug 16, 2016

I'm punting this to 9.1.4.0. So far the only simple case we don't match MRI is when the threading and the proc have the same frame, which is somewhat rare and usually contrived. We need more clarification about this behavior.

@headius headius modified the milestones: JRuby 9.1.4.0, JRuby 9.1.3.0 Aug 16, 2016
@ko1
Copy link

ko1 commented Aug 19, 2016

maybe we need to move to ruby-core or bugs ticket.

anyway:

@ko1 I'm curious...if I said that we should isolate these values across threads, how would you implement that? pthread locals?

To make them isolate between threads, on MRI, maybe I use Hash on each frame, the keys are thraed id and values are $~ contents.

$~ should be $~[Thread.current_thread_id]. More details, we store special store objects on each frame (we named it imemo_svar, s stands for special. $~ is svar->backref. So we can change this code:

  • initialize:svar->backref = {},
  • set: svar->backref[thraed_id] = match_data
  • get: svar->backref[thread_id]

It introduces table reference overhead, but I assume nobody accesses $~ frequently.

@ko1
Copy link

ko1 commented Aug 19, 2016

I agree that isolating them (maybe with $_) between threads is simple, easy to learn. The problem is we can accept the spec change (compatibility issue).

@ko1
Copy link

ko1 commented Aug 19, 2016

Using hash table introduce several problems:

  • if another thread re-use older thread id, it introduces inconsistency.
  • if use Thread value itself instead of thread id, it will introduce critical memory leak.

Clearing all table at the thread object finalization will solve it, but it will be huge cost (we need to traverse all of tables).

@headius
Copy link
Member

headius commented Aug 19, 2016

Read through your proposed idea and I agree they're all problematic.

The threadlocal-in-frame approach works but I'm worried about the cost of cloning the frame state. It's already fairly expensive for JRuby when we need to duplicate the frame...if we had to duplicate thread-local data it would be worse.

And we still haven't addressed the compatibility issue. Even if the threadlocal-on-frame was an acceptable option, it would still break all cases that currently expect to see a $~ across threads. We can't make both scenarios work.

@donv @reidmorrison I am curious about your actual code. I assume it was a case where the proc was created far away from the actual use, and as a result it exhibited the sharing behavior that appears to be the same in MRI. Right?

The all-one-frame example can be made to work because we just mark that frame as "captured" or "escaped" or something, and then every thread creates its own copy. They kinda do that already in JRuby but apparently not enough for this issue. The more general issue of making standalone procs with their own captured frames deal well with threads.

Of course, I'd advocate just getting rid of $~ completely.

@headius
Copy link
Member

headius commented Aug 19, 2016

@ko1 I have filed https://bugs.ruby-lang.org/issues/12689 to continue this discussion.

@ko1
Copy link

ko1 commented Aug 19, 2016

Thanks!

@headius
Copy link
Member

headius commented Nov 8, 2016

No movement on this from MRI and they have similar sharing issues that need to be addressed. Bottom line: on all Ruby implementations up through Ruby 2.4, it is NOT SAFE to use $~ and related variables across threads.

Punting to 9.2. We can revisit this along with the 2.4 work.

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.6.0 Nov 8, 2016
@reidmorrison
Copy link

Would it be worthwhile to add a JRuby switch that completely disables the $~ and related variables?
We consider it bad practice to use them in our application.

@billdueber
Copy link

I've had a user report that this manifests in String#scan as well. I assume any fix will cover this case as well, but wanted to give a head's up.

@headius
Copy link
Member

headius commented May 15, 2018

No movement from MRI. Backref remains thread-unsafe in both JRuby and MRI. Detargeting.

@headius
Copy link
Member

headius commented Jul 19, 2020

I am working on a patch locally that uses our existing captured flag in Frame to switch to thread-local behavior. This avoids having to constantly check if a frame is accessed across threads, since capturing can only be done by the thread that originally owned the frame (via proc, lambda methods or as a result of storing the &block argument).

Once a block has been captured, the frame is marked captured, and subsequent writes will switch to using a ThreadLocal. Reads will continue to return the direct value as long as it is not a ThreadLocal, and then will start returning the thread-local value. The ThreadLocal itself will have an initial value for each thread equal to the value at the time when the first captured write happens.

I believe this will allow all current expectations about behavior to work properly, with minimal added overhead:

  • For uncaptured reads, the nonvolatile read is now volatile and there's a type-check and cast to IRubyObject.
  • For captured reads, the nonvolatile read is now volatile, the IRubyObject type-check will fail, a ThreadLocal type-check will succeed, and a thread-local get will be performed.
  • For uncaptured writes, the captured flag will be found to be false before performing a direct write. Since captured can only be set by the thread that owned the frame originally, until captured a thread can freely write directly to the field.
  • For captured writes, the 'captured' flag will be found to be true before proceeding to set the existing thread-local or atomically switch to using a new thread-local.

headius added a commit to headius/jruby that referenced this issue Jul 19, 2020

Verified

This commit was signed with the committer’s verified signature.
headius Charles Oliver Nutter
Traditionally the $~ and $_ variables have been frame-local but
not thread-local, meaning that if the frame is uprooted by
capturing a block, that frame-local storage is shared across
threads. That leads to issues like jruby#3031, which has been present
in JRuby and CRuby both for at least the past 15 years.

This patch attempts to make writes to backref or lastline on a
captured frame become thread-local without losing the direct
access for simple single-thread accesses.

We already track whether a frame has been captured in order to
eagerly raise LocalJumpError for any non-local breaks or returns
that may originate from that block. This also turns out to be
useful for knowing that the block and its associated frame are now
potentially visible across threads. We reuse this flag to make
backref and lastline safe to read and write across threads.

Once a block has been captured, the frame is marked captured, and
subsequent writes will switch to using a ThreadLocal. Reads will
continue to return the direct value as long as it is not a
ThreadLocal, and otherwise will return the thread-local value. The
ThreadLocal itself will have an initial value for each thread
equal to the value at the time when the first captured write
happens.

I believe this allows all current expectations about behavior to
work properly, with minimal added overhead:

* For uncaptured reads, the nonvolatile read is now volatile and
  there's a type-check and cast to IRubyObject.
* For captured reads, the nonvolatile read is now volatile, the
  IRubyObject type-check will fail, a ThreadLocal type-check will
  succeed, and a thread-local get will be performed.
* For uncaptured writes, the captured flag will be found to be
  false before performing a direct write. Since captured can only
  be set by the thread that owned the frame originally, until
  captured a thread can freely write directly to the field.
* For captured writes, the 'captured' flag will be found to be
  true before proceeding to set the existing thread-local or
  atomically switch to using a new thread-local.

The primary performance issue will most likely be due to the
creation of a ThreadLocal at the moment of the first captured
write. For code sharing procs across threads, this overhead is
unavoidable (and the existing behavior is just a bug that has not
been found yet). However for code using procs alongside backref
or lastline but only on a single thread, this may be a substantial
performance hit with no behavioral improvement.
headius added a commit to headius/jruby that referenced this issue Jul 19, 2020

Verified

This commit was signed with the committer’s verified signature.
headius Charles Oliver Nutter
Traditionally the $~ and $_ variables have been frame-local but
not thread-local, meaning that if the frame is uprooted by
capturing a block, that frame-local storage is shared across
threads. That leads to issues like jruby#3031, which has been present
in JRuby and CRuby both for at least the past 15 years.

This patch attempts to make writes to backref or lastline on a
captured frame become thread-local without losing the direct
access for simple single-thread accesses.

We already track whether a frame has been captured in order to
eagerly raise LocalJumpError for any non-local breaks or returns
that may originate from that block. This also turns out to be
useful for knowing that the block and its associated frame are now
potentially visible across threads. We reuse this flag to make
backref and lastline safe to read and write across threads.

Once a block has been captured, the frame is marked captured, and
subsequent writes will switch to using a ThreadLocal. Reads will
continue to return the direct value as long as it is not a
ThreadLocal, and otherwise will return the thread-local value. The
ThreadLocal itself will have an initial value for each thread
equal to the value at the time when the first captured write
happens.

I believe this allows all current expectations about behavior to
work properly, with minimal added overhead:

* For uncaptured reads, the nonvolatile read is now volatile and
  there's a type-check and cast to IRubyObject.
* For captured reads, the nonvolatile read is now volatile, the
  IRubyObject type-check will fail, a ThreadLocal type-check will
  succeed, and a thread-local get will be performed.
* For uncaptured writes, the captured flag will be found to be
  false before performing a direct write. Since captured can only
  be set by the thread that owned the frame originally, until
  captured a thread can freely write directly to the field.
* For captured writes, the 'captured' flag will be found to be
  true before proceeding to set the existing thread-local or
  atomically switch to using a new thread-local.

The primary performance issue will most likely be due to the
creation of a ThreadLocal at the moment of the first captured
write. For code sharing procs across threads, this overhead is
unavoidable (and the existing behavior is just a bug that has not
been found yet). However for code using procs alongside backref
or lastline but only on a single thread, this may be a substantial
performance hit with no behavioral improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants