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

Attributes created using class_attribute are not thread-safe #26758

Closed
kalsan opened this issue Oct 11, 2016 · 4 comments · Fixed by #29233
Closed

Attributes created using class_attribute are not thread-safe #26758

kalsan opened this issue Oct 11, 2016 · 4 comments · Fixed by #29233

Comments

@kalsan
Copy link

kalsan commented Oct 11, 2016

We are building a large Rails application and ran into concurrency issues. Digging into them, we found that using class_attribute causes a non-thread-safe configuration. With this I don't mean that class_attribute isn't thread-safe - please let me elaborate in more detail:

In issue #14021 @dwbutler initially stated that class_attribute is not thread-safe but then precised: "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." @thedarkone commented: "setting class_attribute at the class level is only supposed to be done at/during boot time". The discussion was later abandoned.
We think that there two different concepts here, getting mixed up: One being that the method class_attribute isn't thread-safe and needs to be done in a single-threaded boot phase. However, even with a clean design respecting this, the program is not going to work, because the setter method generated by class_attribute is not thread-safe either.

A working counterexample can be found in https://gist.github.com/kalsan/cdecebb51830075af7bfdc058a4de1a1

The code mentionned above works fine on Ruby version: 2.3.1, Ruby engine: ruby, ActiveSupport: 5.0.0.1
However, on Ruby version: 2.3.1, Ruby engine: jruby, ActiveSupport: 5.1.0.alpha, the program fails. On my system, failure occurs sometimes with 1'000 and always with 10'000 threads. The error is the following:
NameError: Undefined method setting for 'MyClass'

This is not surprising, since the following code is in class_attribute, line 89:

89: remove_possible_method(name)
90: define_method(name) { val }

This code is not called in the boot phase, but as the attribute :setting is accessed in the multi-threaded running phase. Clearly it is not thread-safe: if an access takes place in between those two lines, the symbol setting is undefined, which explains the above failure.

Again, this does not happen in cruby, possibly due to the GIL story ( link, "But what about the GIL?"). Also, putting a global lock (just for testing!) around the two lines mentioned above makes the problem disappear (of course this ugly workaround is not a solution, but it indicates that the problem is located in there indeed).

Of course, we could use an alternative method in our application for our own calls. However, since Rails itself heavily uses class_attribute, the question arises whether or not Rails is thread-safe on jruby. We have been investigating a lot and found traces indicating that data races might even take place in rails itself.

Feedback and discussion would be much appreciated. We would like to find out how deep this behavour goes and what parts of Rails are affected.

@sudoremo
Copy link
Contributor

We have now come up with a thread-safe but still mutex-free implementation of class_attribute. We're conducting some final tests and will then create a pull request.

@hobodave
Copy link

Any updates on a valid fix for this? Class#class_attributes is used extensively by Rails itself and has concurrency errors under normal usage and operation. e.g. Calling Model.last concurrently will trigger a slew of errors due to the runtime redefinition of methods.

@sudoremo
Copy link
Contributor

As stated in the PR, the singleton class support could potentially be dropped in which case our PR should work just fine I think.

@ankuriitk
Copy link

is there an update on the bug?

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

Successfully merging a pull request may close this issue.

5 participants