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

Already on GitHub? Sign in to your account

class_attribute not thread-safe #14021

Closed
dwbutler opened this Issue Feb 11, 2014 · 14 comments

Comments

Projects
None yet
8 participants
Contributor

dwbutler commented Feb 11, 2014

I'm encountering this issue in a production application with the Paper Trail gem. (See airblade/paper_trail#307). It's running in JRuby/TorqueBox under Rails 3.2.16. The implementation in Rails 4 looks nearly identical.

If I understand the implementation of class_attribute correctly, every time you set the class attribute, it undefines the existing singleton method and defines a new singleton method that simply returns the new value.

In my app, several threads are setting the class attribute of different instances of the same class. This causes the singleton method of the class to be undefined in one thread, and then another thread raises a NameError because the method has been temporarily removed.

It sounds like an unlikely scenario, but this is happening a couple times a day in a relatively low-traffic app.

class_attribute is pretty complex, so I'm not sure how to fix this while retaining the existing functionality. Any advice would be appreciated. Perhaps putting a mutex around the code that undefines/defines would be best?

I have one question about this sentence in specific:

In my app, several threads are setting the class attribute of different instances of the same class.

Why are you doing that? Can you show that on code?

Contributor

dwbutler commented Feb 12, 2014

PaperTrail is doing it in #without_versioning. The current implementation toggles a class attribute back and forth in a thread unsafe manner. So even if class_attribute were fixed to thread-safe, #without_versioning would still not be thread-safe. This is why I have an open pull request that uses a thread local variable instead of a class attribute.

Even though Paper Trail is incorrectly using a class_attribute (IMO), class_attribute should still be fixed. In the worst case, multiple threads should stomp on each others' value. They should not crash with a NameError like they currently do.

Contributor

thedarkone commented Feb 12, 2014

@dwbutler setting class_attribute at the class level is only supposed to be done at/during boot time, I'm not sure your use case is ever going to be supported.

Contributor

dwbutler commented Feb 12, 2014

@thedarkone I'm inclined to agree with you in principle. But...

  1. Nothing enforces this.
  2. This is not well-documented or understood. Even a well-known gem such as PaperTrail is using class_attribute "incorrectly".
  3. I can easily imagine the same thing happening at boot time with threaded servers. If a class_attribute is lazily initialized, and multiple requests hit the server at the same time, the same class_attribute could be set multiple times, resulting in the same error. IMO, class_attribute should be inherently thread-safe so you don't have to worry about such a scenario causing an exception.

@dwbutler dwbutler referenced this issue in airblade/paper_trail Feb 13, 2014

Closed

Thread safety for #without_versioning #328

Contributor

thedarkone commented Feb 13, 2014

I can easily imagine the same thing happening at boot time with threaded servers. If a class_attribute is lazily initialized, and multiple requests hit the server at the same time, the same class_attribute could be set multiple times, resulting in the same error. IMO, class_attribute should be inherently thread-safe so you don't have to worry about such a scenario causing an exception.

Rails multithreading strategy depends on boot time being single threaded, while also requiring that all code/class loading/initializing happens exclusively during boot time.

Contributor

dwbutler commented Feb 14, 2014

I agree that boot time should be single-threaded and that initialization should only happen during boot time.

However, not every gem author will conform to this standard so strictly. It's also possible that some configuration may need to change at runtime.

I also disagree that class_attribute is only used for configuration/initialization purposes. Quoting the documentation, class_attribute is used to "Declare a class-level attribute whose value is inheritable by subclasses". One would naturally expect that a "class-level attribute" can and will be changed at any time during run-time.

Since class_attribute can be used anywhere for any purpose, I strongly believe that it should be fixed to be thread-safe.

Contributor

printercu commented Feb 15, 2014

I also thought about thread-safe implementation of class_attribute. But I found two expected behaviours:

  • add Mutex#synchronize into writer in current implementation, and when you change attr in one thread it'll be changed in all threads (as now, but thread-safe);
  • implement it with Thread#instance_variables, and changing attr's value will have affect only in current thread.

There is a lot of libraries that use attributes similar to second case for runtime configuration, which makes them easy to use (ex. connection params in ActiveResource). Maybe we can add second case as extension with option threaded: true.

There is also workarounds like this rails/activeresource#61, and I think it's not the latest.

I can prepare PR.

Contributor

dwbutler commented Feb 17, 2014

@printercu That would be great! I think implementing both behaviors would be a good step forward.

@dwbutler dwbutler added the stale label May 27, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Contributor

dwbutler commented May 29, 2014

Reproduced in ActiveSupport 4.1.1. See the following Gist: https://gist.github.com/dwbutler/2c5bc4b7f05e5f7ecfd9

Contributor

dwbutler commented May 29, 2014

@printercu Have you thought more about this issue?

add Mutex#synchronize into writer in current implementation, and when you change attr in one thread it'll be changed in all threads (as now, but thread-safe);

This behavior seems sensible to me. It would be ideal to keep the behavior of class_attribute consistent (i.e. all threads share the same value).

Contributor

printercu commented May 29, 2014

For now you can just use Mutex.synchronize {} in your code. I'll try to take a look on the code soon.

@rafaelfranca rafaelfranca removed the stale label Aug 19, 2014

Any update on the bug?

@matthewd matthewd closed this in #29233 Sep 1, 2017

awesome @matthewd : I presume PR #29233 fixes this. When will this be released? Will it be part of Rails 5.0.6 (I see a release candidate for 5.0.6) or a later version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment