Thread safety for #without_versioning

[dwbutler]

This fixes #326 and #307.

The thread-local paper_trail_store is now used to track whether Paper Trail is enabled for a particular model. Note that this results in a small API change. Model.paper_trail_enabled_for_model is now set at runtime and doesn't change. (Changing the value of a class_attribute is not thread-safe; see #307). PaperTrail.enabled_for_model?(Model) is now the preferred way to query if Paper Trail is enabled. If not specified for the current thread, it falls back to querying Model.paper_trail_enabled_for_model.

I considered trying to leave the API unchanged, but it's very difficult to mimic all the behaviors of class_attribute without using class_attribute.

I would appreciate any feedback! I'm not sure how "public" the Model.paper_trail_enabled_for_model API is.

[batter]

@dwbutler - Thanks for the PR. I'm trying to wrap my head around this, as I don't deal with multithreaded Ruby/JRuby very often. When you say that paper_trail_enabled_for_model is not 'thread safe', what exactly do you mean by 'thread safe'? Looking at your example test provided on your PR:

slow_thread = Thread.new do
  Widget.new.without_versioning do
    sleep(0.01)
    enabled = PaperTrail.enabled_for_model?(Widget)
    sleep(0.01)
  end

  enabled
end

fast_thread = Thread.new do
  sleep(0.005)
  enabled = PaperTrail.enabled_for_model?(Widget)
  enabled
end

Are you suggesting that enabled_for_model? in the fast_thread should return a value completely isolated from what is returned in the slow_thread? If I'm not missing something, I believe that in an MRI instance (where everything is technically single threaded anyways), this is not the case, because you are invoking without_versioning in the slow_thread, and then sleeping for 0.02 within that block, which basically means:

Widget.paper_trail_enabled_for_model = false
sleep(0.02)
Widget.paper_trail_enabled_for_model = true

So if I'm not mistaken, until that time has expired, and that without_versioning block exits back out, Widget.paper_trail_enabled_for_model will return false (even if queried from a different thread running simultaneously) until that method has exited back out. Am I missing something here?

[dwbutler]

Hi @batter,

The problem I'm trying to solve is that #without_versioning does not work as expected when called from multiple threads. The thread that called #without_versioning should not turn off versioning in other threads. Currently it does. Also, it causes random failures because class_attribute is not thread-safe. (See the referenced rails/rails#14021 for details.)

I did not attempt to fix Widget.paper_trail_enabled_for_model because it's impossible to turn this on and off in a thread-safe fashion (since it's a class_attribute). If you're okay with moving away from using class_attribute, we could keep using Widget.paper_trail_enabled_for_model.

Just to clarify, MRI is indeed truly multi-threaded, and anyone who runs a threaded server (such as Puma) will experience the same issue. There is a global interpreter lock, but that will not protect against this problem.

[batter]

@dwbutler - I tried switching paper_trail_enabled_for_model to use attr_accessor and then running the tests you submitted with your pull, and it still didn't seem to be able to hold state outside of a single thread, so I'm wondering if attr_accessor isn't thread safe either, but I assumed it would be as per our discussions that all core features in the Ruby library should be thread safe.

[dwbutler]

@batter - I also tried attr_accessor but I found it difficult to replicate the functionality of class_attribute. Did you use an attr_accessor for the instance or for the class?

class Widget
  # Will only hold its value for a single instance
  attr_accessor :paper_trail_enabled_for_model

  class << self
    # Will hold its value for all instances of the class
    attr_accessor :paper_trail_enabled_for_model
  end
end

In regards to thread safety, it's "safe" in the sense that it uses instance variables under the hood and so there shouldn't be any errors. But it could still be "unsafe" in the sense that setting a class level attribute will make the change visible in other threads.

The only way to correctly turn paper trail on and off per-thread is to either use instance variables, or thread-local variables. Hope that made sense.

[batter]

@dwbutler - Here is the work that I did in an attempt to move away from class_attribute implementation for that method on a branch I just pushed so you can take a look. Here's the branch.

It looks like your solution may be the preferred one since it actually works properly. I'd still like to maintain the paper_trail_enabled_for_model? accessor method at a class level on a model, but it can merely be a shortcut that points to the paper_trail_store. I'm just wondering if you might be able to cue me in as to what I might be doing in that implementation that is not thread safe since I was using an instance variable to store the value there.

[dwbutler]

@batter - Just took a look at your branch. I can see that you replicated the functionality that class_attribute provides by using a class-level instance variable.

The problem with this approach is that all threads "see" the same value for paper_trail_enabled_for_model. And if two threads call without_versioning at the same time, versioning could accidentally get turned off permanently.

I think your approach of keeping the same API is cleaner, and just needs to be modified to read from the thread-local paper_trail_store.

[batter]

Ahh ok, so basically the only variables you can assume are thread-safe / independent per thread are class variables (indicated by @@ preceding the name)? Then your original PR looks like the most logical way to go on this one. I'll do some minor modifications to clean it up as discussed here and try to get it merged in soon. Thanks for all the guidance!