Match data missing in concurrent proc #3031
Comments
|
From IRC #jruby: |
|
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) |
|
There does appear to be a behavioral difference from MRI here: 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. |
|
This is likely causing your issue, @reidmorrison. |
|
@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 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. |
|
@reidmorrison I don't think the |
|
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. |
|
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: |
|
@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. 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? |
|
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): |
|
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: |
|
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. |
|
maybe we need to move to ruby-core or bugs ticket. anyway:
To make them isolate between threads, on MRI, maybe I use Hash on each frame, the keys are thraed id and values are $~ contents.
It introduces table reference overhead, but I assume nobody accesses |
|
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). |
|
Using hash table introduce several problems:
Clearing all table at the thread object finalization will solve it, but it will be huge cost (we need to traverse all of tables). |
|
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. |
|
@ko1 I have filed https://bugs.ruby-lang.org/issues/12689 to continue this discussion. |
|
Thanks! |
|
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 Punting to 9.2. We can revisit this along with the 2.4 work. |
|
Would it be worthwhile to add a JRuby switch that completely disables the |
|
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. |
|
No movement from MRI. Backref remains thread-unsafe in both JRuby and MRI. Detargeting. |
|
I am working on a patch locally that uses our existing 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:
|
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.
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.
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?
The text was updated successfully, but these errors were encountered: