Fork me on GitHub

February 2, 2012

Preventing Recursion in Ruby (without Thread.current)

Post moved to http://log2.kares.org/post/59891248632/preventing-recursion-in-ruby-without-thread-current

This post is inspired by Preventing Recursion in Ruby.

First a confession: every time I see Thread.current[] I start thinking of ways to eliminate it. Thus I believe there's at least one thing wrong with the approach presented in the post. Let's try rebuilding prevent_recursion :


It seems to serve almost the same purpose and using an object's singleton class just feels right. Please note, that this does not behave exactly the same way compared to the threaded solution - we prevented a method from recursing once (or "forever" if we remove the (class << self) line), however this should most likely suffice.

Of course, I only got "smart" by failing previously, but since than I always think twice over storing anything in Thread.current, which is one of the most abused patterns in Ruby and no matter how uniquely you name those variables it's still a smell. Maybe it's about time thread-safe frameworks, such as Rails, should provide us with an abstraction for safely storing thread local data.

Now, I should probably end this right here - point proven. But since the post mentioned infinite recursion with ActiveRecord::Callbacks I immediately remembered my very own case where I would be of no luck using the above approach. Try the following sample :


A real world scenario would be keeping an array-like index for items in a basket. Each basket has it's items ordered and changing an index of an item "swaps" it with the other item at the given position. One way of doing it, which requires a recursion prevention would be :


Notice how the method gets redefined with a dummy one to prevent recursion.

I'm a huge fan of doing things DRY, on the other hand sometimes it might be contra-productive, especially when it can't cover all cases or is only used once or twice. Not to mention how much easier it would be for a new comer to read a method such as assign_index and understand the trick immediately then having to dive deep into another module's meta-stuff.

UPDATE: As @avdi pointed me out, I realized I'm actually promoting non-thread-safe code. I narrowed my recursion prevention domain to short lived AR instances that are never accessed beyond a single request, however this is clearly not a completely valid assumption to make. Especially since the original prevent_recursion targets a much wider audience.
Notice the difference in behavior if the prevented (thread-safe) instance method or a shared object is accessed by two concurrent threads, Thread.current will guarantee a correct result for each caller, however my instance_eval approach will not. And so the moral of this story seems that a slice of Thread.current[] "pollution" to prevent recursion is inevitable after all.

7 comments:

  1. Your solution is not actually thread-safe.

    Of course in a normal Rails application/workflow it is unlikely that two different threads would have a reference to the same object, but it is still possible.

    ReplyDelete
  2. @caradoc right, but this is not the point - in a GIL free Ruby the Thread.current approach over the same object might end up being unpredictable as well.

    ReplyDelete
  3. Please explain how being gil-free would make the threadsafe version unpredictable?

    ReplyDelete
  4. @avdi since there's no locking two concurrently executing threads might pass through the very first line `return if Thread.current[flag_name]` thus calling "original" twice.

    Mine in this highly concurrent context is no better and I thought this post is not so about the threadsafe! details but about avoiding recursive callbacks.

    As a JRuby adopter I certainly would not want to encourage non-threadsafe code (my AR instances are never accessed from multiple threads). I also meant no dis-respect towards your post.
    I only wanted to advocate non Thread.current solutions whenever possible.

    ReplyDelete
  5. That's irrelevant. The use of a thread-local is only to prevent recursion in a single thread of execution - if it prevented two two threads from passing through the first line concurrently it would be *broken*. We're not protecting a shared resource here, we're preventing recursion in a given stack.

    Speaking of stacks, your version is not Fiber-safe either; which moving forward is particularly dangerous moving forward, since even single-threaded programs may use Fibers to simplify control flow (e.g. in Reactor-based systems).

    ReplyDelete
  6. Blah, sorry about the grammar. Typing too fast.

    ReplyDelete
  7. Oh, I'm sorry I was looking today at a huge synchronize block over a "shared" method for a while and it obviously stuck in my head ... of course it would be broken if it did not work.

    I basically just thought looking at the original post that an instance's recursion seems to be manageable by the very instance only.
    Guess I haven't think it entirely through esp. the consequences of two threads attempting to call the same method (since I basically assumed short-lived AR objects) - Thread.current definitely gives a more consistent behavior.

    ReplyDelete