Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Thread safety for #without_versioning #328

Closed
wants to merge 2 commits into from

2 participants

@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.

@dwbutler dwbutler referenced this pull request in rails/rails
Open

class_attribute not thread-safe #14021

@batter
Collaborator

@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
Collaborator

@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
Collaborator

@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
Collaborator

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!

@batter batter added this to the 3.0.1 milestone
@batter batter self-assigned this
@batter batter added Confirmed Bug and removed Confirmed Bug labels
@batter batter closed this pull request from a commit
@batter batter close #307, close #326, close #328; Make Model.paper_trail_enabled_fo…
…r_model? thread-safe
0359704
@batter batter closed this in 0359704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
10 lib/paper_trail.rb
@@ -32,6 +32,16 @@ def self.enabled_for_controller?
!!paper_trail_store[:request_enabled_for_controller]
end
+ # Sets whether PaperTrail is enabled or disabled for this model in the current request.
+ def self.enabled_for_model(model, value)
+ paper_trail_store[:"request_enabled_for_#{model}"] = value
+ end
+
+ # Returns `true` if PaperTrail is enabled for this model in the current request, `false` otherwise.
+ def self.enabled_for_model?(model)
+ !!(paper_trail_store.fetch(:"request_enabled_for_#{model}", model.paper_trail_enabled_for_model))
+ end
+
# Set the field which records when a version was created.
def self.timestamp_field=(field_name)
PaperTrail.config.timestamp_field = field_name
View
16 lib/paper_trail/has_paper_trail.rb
@@ -81,12 +81,16 @@ def has_paper_trail(options = {})
# Switches PaperTrail off for this class.
def paper_trail_off
- self.paper_trail_enabled_for_model = false
+ PaperTrail.enabled_for_model(self, false)
end
# Switches PaperTrail on for this class.
def paper_trail_on
- self.paper_trail_enabled_for_model = true
+ PaperTrail.enabled_for_model(self, true)
+ end
+
+ def paper_trail_enabled_for_model?
+ PaperTrail.enabled_for_model?(self)
end
def paper_trail_version_class
@@ -194,11 +198,11 @@ def next_version
# Executes the given method or block without creating a new version.
def without_versioning(method = nil)
- paper_trail_was_enabled = self.paper_trail_enabled_for_model
- self.class.paper_trail_off
+ paper_trail_was_enabled = PaperTrail.enabled_for_model?(self.class)
+ PaperTrail.enabled_for_model(self.class, false)
method ? method.to_proc.call(self) : yield
ensure
- self.class.paper_trail_on if paper_trail_was_enabled
+ PaperTrail.enabled_for_model(self.class, paper_trail_was_enabled)
end
private
@@ -323,7 +327,7 @@ def changed_and_not_ignored
end
def paper_trail_switched_on?
- PaperTrail.enabled? && PaperTrail.enabled_for_controller? && self.paper_trail_enabled_for_model
+ PaperTrail.enabled? && PaperTrail.enabled_for_controller? && PaperTrail.enabled_for_model?(self.class)
end
def save_version?
View
4 test/dummy/app/controllers/test_controller.rb
@@ -1,5 +1,5 @@
class TestController < ActionController::Base
- def user_for_paper_trail
- Thread.current.object_id
+ def current_user
+ @current_user ||= OpenStruct.new(:id => Thread.current.object_id)
end
end
View
26 test/functional/thread_safety_test.rb
@@ -1,7 +1,7 @@
require 'test_helper'
class ThreadSafetyTest < ActionController::TestCase
- test "be thread safe" do
+ test "be thread safe when using #set_paper_trail_whodunnit" do
blocked = true
slow_thread = Thread.new do
@@ -23,4 +23,28 @@ class ThreadSafetyTest < ActionController::TestCase
assert_not_equal slow_thread.value, fast_thread.value
end
+
+ test "be thread safe when using #without_versioning" do
+ enabled = nil
+
+ 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
+
+ assert_not_equal slow_thread.value, fast_thread.value
+ assert Widget.paper_trail_enabled_for_model
+ assert PaperTrail.enabled_for_model?(Widget)
+ end
end
View
2  test/unit/model_test.rb
@@ -495,7 +495,7 @@ def without(&block)
context 'when destroyed "without versioning"' do
should 'leave paper trail off after call' do
@widget.without_versioning :destroy
- assert !Widget.paper_trail_enabled_for_model
+ assert !PaperTrail.enabled_for_model?(Widget)
end
end
Something went wrong with that request. Please try again.