Ruby core classes aren't thread safe

http://www.jstorimer.com/newsletter/ruby-core-classes-arent-thread-safe.html

by bitshift • 13 years ago

37 43 13 years ago

wuest• 13 years ago

Nice write-up. It's easy to fall into the trap of assuming that things are "thread-safe" when writing under the iron fist of a Global Interpreter Lock.

Ruby threads seems to be a fairly narrow topic on which to base a book; I'll look forward to seeing what all the book covers.

niggler2• 13 years ago

Does the spec mandate thread safety? (I guess I should ask if there's a spec or is MRI the reference implementation)

jstorimer3• 13 years ago

AFAIK there is no spec. MRI is the reference implementation, but many things are experimental or intentionally unspecified.

Given that MRI ships with a GIL, the only core classes that are intentionally aware of multi-threading concerns are Mutex, ConditionVariable, and Queue.

masklinn11• 13 years ago

Even a thread-aware collection where collection methods are synchronized on an internal lock (as in Java 1.0 — this was quickly dropped as it's effectively useless) wouldn't help here: having `[]` and `[]=` safe will not make calling `[]`, performing an addition and then calling `[]=` safe.

aardvark1793• 13 years ago

A GIL does not mean classes should ignore concurrency concerns, it's still possible to get odd behaviour from things like hash table implementations in a GIL based interpreter when inserting objects as you may end up thread switching mid operation.

What saves you most of the time is that it isn't worth switching threads too often so normally you get lucky.

steveklabnik• 13 years ago

There is an ISO spec, but it's not really relevant to the future of the language.

exabrial• 13 years ago

Does ruby have a spec? If not, I don't really see this as a problem... synchronization, especially in a dynamic language that's already really slow, would just add more overhead.

chc• 13 years ago

It does, though it's the sort that's mainly derived from the canonical implementation -- see RubySpec.

pieter• 13 years ago

This still doesn't explain why the MRI implementation is accidentally threadsafe. Why doesn't the interpreter switch threads after reading the value from the hash but before storing the updated value?

sabat2• 13 years ago

Due to the Global Interpreter Lock (GIL), your whole script is wrapped in one giant mutex. That means that you don't have code running in true parallel, so the data is not corrupted as it is in JRuby and Rubinius (which both implement real, true, parallel threads).

sabat2• 13 years ago

Not sure why someone chose to downvote this to 0, but here's how the OP put it:

The global lock is a feature of MRI that basically wraps a big mutex around all of your code. That's right, even if you're using multiple threads on a multi-core CPU, if your code is running on MRI it will not run in parallel.

tomp3• 13 years ago

That doesn't explain it. The runtime execution could still be like this:

  thread #1:
    local a = x[i]
    a = a + 1
    context_switch()

  thread #2:
    local b = x[i]
    b = b + 1
    x[i] = b
    context_switch()

  thread #1:
    x[i] = a

resulting in a wrong (or merely unexpected) result.

masklinn11• 13 years ago

If it's like CPython, the interpreter will release the GIL on IO or after a number of "ticks", but "ticks" are very loosely mapped to an expression or statement. So it will generally put all of (get value at index, increment value, store value) under a single lock.

So I'd expect that even on MRI the original code could in fact fail to yield 0 once in a while. That's what'd happen on CPython

masklinn11• 13 years ago

Under the semantics described here, Java or C# core classes aren't "thread-safe" either and I'd expect the vast majority of standard libraries to completely fail the test (potential winners: Clojure using an immutable collection bound on an atom, as they have compare-and-swap semantics; and probably Haskell somehow), the example code requires performing the following actions atomically:

* Loading an instance-local collection

* Fetching a numeric value from the collection

* Incrementing or decrementing the numeric value

* Putting the incremented value back

Even if the collection is "synchronized" (each method call takes a collection-local lock), because the value is altered outside the collection there's no way for the change to be atomic unless it's wrapped in a transaction block or protected by a lock. As far as I can think, the only ways for core classes to "be thread-safe" considering the example (keeping mutable collections semantics) would be to either have collections dedicated specifically to the operation such as Java6's AtomicIntegerArray (http://docs.oracle.com/javase/6/docs/api/java/util/concurren...), or to have the collection apply the operation internally e.g. Hash#apply(key, &operation) used roughly like this:

    def decrease item
      @stock.apply(item) {|from| from - 1}
    end

dons2• 13 years ago

> and probably Haskell somehow

By design - pure values are always thread safe.

masklinn11• 13 years ago

Pure values are not really relevant: you still need to update a binding somewhere to synchronize, and that's sufficient for your race. The clojure example wouldn't be safe if atoms weren't compare-and-set: have collection state A, thread one applies A->B, thread two applies A->C, the two threads set the atom (atomically but not CAS) and an increment has been lost even though all values are pure.

aardvark1793• 13 years ago

A better title might have been, "true concurrent execution may reveal problems in your code that a GIL has hidden."

Having worked on a JVM port of a similar language and standard library what is most surprising is how much will still work in real world situations even when fundamental parts are not thread safe. It requires careful insertion of locks, and some quite heavy multithreaded testing to find and fix those problems, and that time may be better spent implementing new thread safe alternatives (or simply exposing the java concurrent collections) which encourage the programmer to write thread safe code, and putting locks in at the application level to fix concurrency issues.

masklinn11• 13 years ago

> A better title might have been, "true concurrent execution may reveal problems in your code that a GIL has hidden."

Indeed (and as many Python developers have discovered trying to run their code on pypy, deterministic "garbage collection" schemes such as refcounting may do the same with resource leaks)

badgar3• 13 years ago

> Under the semantics described here, Java or C# core classes aren't "thread-safe" either and I'd expect the vast majority of standard libraries to completely fail the test (potential winners: Clojure using an immutable collection bound on an atom, as they have compare-and-swap semantics; and probably Haskell somehow)

Java's standard library includes Doug Lea's famous java.util.concurrent package written as part of JSR 166, which according to (http://jcp.org/en/jsr/detail?id=166) was finally released in late 2004. Never wrote C#, but I started typing [C# concurrent] into Google and it autocompleted and instantly searched for [C# ConcurrentDictionary], the first result being: http://msdn.microsoft.com/en-us/library/dd287191.aspx .

masklinn11• 13 years ago

> Java's standard library includes Doug Lea's famous java.util.concurrent package written as part of JSR 166

Irrelevant, java.util.concurrent.atomic.AtomicIntegerArray - which I mentioned — can not under any sensible definition be called a "core collection".

tomp3• 13 years ago

It's not that Arrays are not thread safe; it's just that the code was written in a non-thread-safe way.

Writing

  x[i] -= 1

actually means

  x[i] = x[i] - 1

So, there's a read, a subtraction, and a write, and they all happen sequentially. Since they are not in a transaction or protected by a mutex, nothing guarantees that other thread don't mutate `x[i]` in the mean time.

This has nothing to do with Ruby, and nothing to do with multicore, either. Even on a single CPU core, threads might interleave and cause unexpected behavior.

SeoxyS• 13 years ago

I came to this comment thread specifically to point this out, but you beat me to it.

It has nothing to do thread safety, and everything to do with atomicity. This is not a single atomic operation, but rather three atomic (and thread-safe) operations which are bound together with the assumption that the entire thing is atomic when it is not.

    # you might as well imagine this happening
    original = x[i]
    new_val = original - 1
    x[i] = new_val

Which is why you need to use a mutex to make the entire operation atomic, at a small performance cost.

The Objective-C compiler actually adds a nice bit of syntax for this, you can simply wrap your code in @synchronized:

    @synchronized(self) {
         int original = x[i];
         int new_val = original - 1;
         x[i] = new_val;
    }

danabramov• 13 years ago

If I understand correctly, this corresponds to `lock` in C#:

    lock (_gate) {
        x[i] --;
    }

At least in C# it is considered preferable to lock on a private field, as opposed to locking on `this`, so nobody else also locks on your instance, potentially causing a deadlock.

I suppose this applies to `@synchronized` in Objective C as well.

I like that .NET Framework also provides some useful atomic methods, including this one:

    Interlocked.Decrement (ref x[i]);

They come in handy.

niggler2• 13 years ago

That's not quite true. The language spec can mandate that the -= be atomic (the X86 equivalent is mandating a LOCK).

masklinn11• 13 years ago

Technically it'd have to mandate that []-= be atomic, there's a load from and a store to a k:v collection, not just from and to memory.

zopticity• 13 years ago

Totally agreed. Why do I have to lock it myself (even on a high level language like Ruby)? I'm not programming for C!

incidently• 13 years ago

The article seems to be wrong in several aspects ... First, the issue described has nothing to do with arrays; the same problem happens when using a plain number:

    class Inventory
      def initialize(nb)
        @nb_items = nb
      end
     
      def decrease
        @nb_items -= 1
      end
     
      def nb_items
        @nb_items
      end
    end 

    @inventory = Inventory.new(4000) 


    threads = Array.new
    400.times do
      threads << Thread.new do
        10.times do
          @inventory.decrease
        end
      end
    end
     
    threads.each(&:join)
    puts @inventory.nb_items

Second, the mutex in the OP's code synchronizes the whole block passed to a thread, i.e. there's no parallelism at all (the second thread waits until the first one finishes, and so on). It should rather be something like:

    class Inventory
      def initialize(nb)
        @nb_items = nb

        @lock = Mutex.new
      end
     
      def decrease
        @lock.synchronize do
          @nb_items -= 1
        end
      end
     
      def nb_items
        @nb_items
      end
    end 

    @inventory = Inventory.new(4000) 

    threads = Array.new

    400.times do
      threads << Thread.new do
        10.times do
          @inventory.decrease
        end
      end
    end
     
    threads.each(&:join)
    puts @inventory.nb_items

jstorimer3• 13 years ago

Hi. OP here.

Thanks for the comments about atomicity vs. thread-safety. Absolutely on point. The article started out demonstrating what happened with concurrent Array mutation, but then I put in that += operation and didn't address it. Sorry for not making the distinction. Atomicity is absolutely a different issue than a thread-safe collection. I'm publishing something new tomorrow that addresses this point.

To bring things back to code, the point I was originally trying to make is that this code is not thread-safe.

  array = []
  threads = []

  10.times do
    threads << Thread.new do
      100.times { array.push(rand) }
    end
  end

  threads.each(&:join)
  # 10 threads each inserted 100 values, result should be 1000
  puts array.size

Specifically, too many Ruby programmers won't think twice about this operation not being thread-safe:

  array.push(item)

But there's no such guarantee. This is demonstrated nicely when this code example is run on an implementation with no global lock, try it on JRuby.

JulianMorrison• 13 years ago

There is one core library class that's thread safe, and it's Queue.

Otherwise, take a lock, or don't share data.

marshray• 13 years ago

Let's ban this term "thread safe" and instead say what we really mean.

But first, let's figure out what we really mean.

Ruby core classes aren't thread safe

Related Stories