PaperTrail always enabled in controllers.

[randomor]

I'm using Puma as my web server, and have in application.rb:

    config.after_initialize do
      #run `spring stop` before change takes effect.
      PaperTrail.enabled = false
      # do this again just to be sure I've set it
      PaperTrail.paper_trail_store[:paper_trail_enabled] = false
    end

I'm able to login to rails c and see: PaperTrail.enabled? #=> false

However, when I'm posting a request to one of my endpoint with binding.pry, and when I do PaperTrail.enabled? #=> true I've created an app from scratch and it's running webrick and still has the same issue. Is this a bug or am I missing something?

[jaredbeck]

Can you reproduce this issue with the latest version of PaperTrail (currently 4.0.0)?

What does your endpoint look like?

I've created an app from scratch and it's running webrick and still has the same issue.

Can you submit a PR with a failing test that demonstrates the issue?

[randomor]

Sorry, I am not able to submit a PR with failing test with this depository, due to my unfamiliarity with the codebase and lack of time. And I have since used a custom solution to track changes for some areas of the app. But here is my sample app: https://github.com/randomor/paperapp/blob/master/app/controllers/posts_controller.rb#L12
And this line will always return true.

[jaredbeck]

I tried out your sample app (version fa77aae3c). It works for me. Here are the steps I took:

git clone git@github.com:randomor/paperapp.git
bundle
bundle exec db:setup
bundle exec rake test

Produces the following output:

Run options: --seed 38635

# Running:

...>>>>>>>>>>>>> false
....

Finished in 1.286454s, 5.4413 runs/s, 10.1053 assertions/s.

I wonder what we are doing differently? Any ideas?

[randomor]

Yes. Test will pass. But if you do rails s and try hit the route you will see it log false.

[jaredbeck]

Yes. Test will pass. But if you do rails s and try hit the route you will see it log false.

You mean true, right? :) Yeah, it does.

[2015-10-16 22:18:25] INFO  WEBrick::HTTPServer#start: pid=66091 port=3000
Started GET "/posts" for 127.0.0.1 at 2015-10-16 22:18:36 -0400
  ActiveRecord::SchemaMigration Load (0.1ms)  SELECT "schema_migrations".* FROM "schema_migrations"
Processing by PostsController#index as HTML
>>>>>>>>>>>>> true

Let me dig around and see if there's any explanation for that. It definitely seems wrong.

[randomor]

Sorry. Yes. That's what I meant. Thanks!

On Fri, Oct 16, 2015 at 9:20 PM, Jared Beck notifications@github.com
wrote:

Yes. Test will pass. But if you do rails s and try hit the route you will see it log false.
You mean true, right? :) Yeah, it does.

[2015-10-16 22:18:25] INFO  WEBrick::HTTPServer#start: pid=66091 port=3000
Started GET "/posts" for 127.0.0.1 at 2015-10-16 22:18:36 -0400
  ActiveRecord::SchemaMigration Load (0.1ms)  SELECT "schema_migrations".* FROM "schema_migrations"
Processing by PostsController#index as HTML
>>>>>>>>>>>> true

Let me dig around and see if there's any explanation for that. It definitely seems wrong.

Reply to this email directly or view it on GitHub:
#635 (comment)

[watsonbox]

Perhaps I can shed some light on this. PR #541 introduces a 'thread safe' PaperTrail.enabled to resolve issue #539. What this really is (using request_store) is a thread local which gets cleared on every request. This means that if you want to set it outside of a request, it won't work.

Furthermore, this means that disabling PaperTrail to speed up tests is broken too for any request-level tests. I get around this by doing something like the following to disable the thread local implementation in my test suite:

module PaperTrail
  private

  def self.paper_trail_store
    @paper_trail_store ||= { :request_enabled_for_controller => true }
  end
end

It looks like this could do with a bit more attention.

[jaredbeck]

Thanks Howard, that's helpful. Given that PaperTrail.enabled was deliberately made thread-local in #541, I don't think we'll want to change it back to global.

So, I guess we should look for a way to at least disable PT in test env. Any ideas?

[seanlinsley]

@jaredbeck I don't see very much discussion on the pros and cons of making it thread-local. Perhaps @batter at the time hadn't considered this sort of scenario?

If we can switch back to it being a global variable, a thread-safe implementation is simple to create:

def initialize
  @mutex = Mutex.new
end

def enabled
  @mutex.synchronize do
    @enabled
  end
end

def enabled=(enabled)
  @mutex.synchronize do
    @enabled = enabled
  end
end

[jaredbeck]

@jaredbeck I don't see very much discussion on the pros and cons of making it thread-local.

Neither do I, but I haven't looked super-hard.

If we were to change it back to "global" (technically, a singleton ivar) we'd still have paper_trail_enabled_for_controller as a way to disable PT on a per-request basis, wouldn't we? So, I'd be open to that unless Ben sees a problem.

[batter]

@watsonbox - Thanks for chiming in here, I see what you're saying. When you overrode it to disable it in your test suite did you actually set request_enabled_for_controller: false?

I've been thinking about this and I'm wondering whether we could have some sort of way to have an optional initializer for the gem which loads up an key value store of the initial state of the paper_trail_store, and after load becomes immutable, and from that point forward, the request_store will always reset back to that state after a request is made. I will see if I can make a feature branch.

[jaredbeck]

I will see if I can make a feature branch.

Hi Ben, looks like we're both thinking about this tonight. You may want to review #717 first, or use it as a starting point, or whatever. I'm looking forward to seeing what you come up with too, or any suggestions you have.

[batter]

@jaredbeck - Thanks, that looks neat, I will have to read that more closely tomorrow when I'm not so sleepy, but that looks like a promising start. I wish I was more knowledgable about working with threading to be honest.

[seanlinsley]

@batter can you comment on this? #635 (comment)

[seanlinsley]

Moving it back to a "global" variable with a thread-safe implementation should make #717 unnecessary

[jaredbeck]

Moving it back to a "global" variable with a thread-safe implementation should make #717 unnecessary

A single unsafe variable would certainly be simpler than #717, and I'm a big fan of simpler.

Regarding the term "thread-safe", I would expect a "thread-safe" implementation to avoid the following error:

1. Thread A sets enabled = true
2. Thread B sets enabled = false
3. Thread A reads enabled, gets false

From thread A's perspective is this actually "thread-safe"? I am not convinced it is. The implementation suggested above (#635 (comment)) is susceptible to this error.

[seanlinsley]

Why would different threads want it set differently in the first place? That's what I don't understand, that wasn't explained in #541.

[seanlinsley]

And to clarify on what "thread-safe" means, it means that there would never be a race condition if two threads tried reading or writing to the variable at the same moment. The mutex puts a lock on the variable, so if two threads try accessing it at the same time, they happen serially instead of simultaneously.

https://en.wikipedia.org/wiki/Thread_safety

Something can be thread-safe without providing thread-local variables.

[jaredbeck]

Why would different threads want it set differently in the first place?

I don't have a good answer for this, actually. I was thinking about threaded servers like puma, and that people might want to disable PT for an entire thread, but in reality PaperTrail.enabled_for_controller= should work fine for that use case.

[jaredbeck]

Closed by #723, a breaking change, scheduled for 5.0 release.