Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable observers without cross-thread side effects #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

markmeeus
Copy link

The current implementation of enabling/disabling observers has side effects into other threads.
I understand that the set of observers per model should be global, but disabling an observer is usually done for a specific reason and a specific scope.

When doing this:

    Heisenberg.observers.disable :uncle_frank do
        #code with observer disabled
    end

Other threads using the Heisenberg model will have the observer disabled until the block is done with it.

This pull requests doesn't change the global behaviour of observers, only the disabling/enabling is per thread.

@rafaelfranca
Copy link
Member

Thank you. Since we are using Rails 4 could you change your code to use ActiveSupport::PerThreadRegistry

@markmeeus
Copy link
Author

Hi @rafaelfranca ,

I didn't know about that Registry class, but I implemented it in my last commit.

Since these PerThreadRegistries are singletons, I had to use 1 registry for all instances of observer_array.
That's why it's disabled_observers_per_class[]

@simonoff
Copy link

Any updates?

@stormsilver
Copy link

👍 this is a problem for us in production and our current solution is to fork whenever we need to disable callbacks. :(

If you still would like to see changes to this and @markmeeus isn't available or doesn't have time to make them I would be glad to take over the PR.

@stormsilver
Copy link

As an update, we have been running this code in production for many many months now with no problems.

@markmeeus
Copy link
Author

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants