Fix `thread_mattr_accessor` share variable superclass with subclass #25681

willnet · 2016-07-04T12:28:26Z

Summary

The current implementation of thread_mattr_accessor set variable
sharing superclass with subclass. So the method doesn't work as documented.

Precondition

class Account
  thread_mattr_accessor :user
end

class Customer < Account
end

Account.user = "DHH"
Account.user  #=> "DHH"
Customer.user = "Rafael"
Customer.user # => "Rafael"

Documented behavior

Account.user  # => "DHH"

Actual behavior

Account.user  # => "Rafael"

Current implementation set variable statically likes Thread[:attr_Account_user],
and Customer also use it.

Make variable name dynamic to use own thread-local variable.

rails-bot · 2016-07-04T12:28:29Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @senny (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

senny · 2016-07-25T15:26:27Z

activesupport/test/core_ext/module/attribute_accessor_per_thread_test.rb

-      thread_mattr_reader   :shaq, instance_reader: false
-      thread_mattr_accessor :camp, instance_accessor: false
-    end
+  class Foo


Are anonymous classes not sufficient to reproduce the issue? Would:

@class = Class.new do end @subclass = Class.new(@class) do end

not work?

Anonymous classes don't work with thread_mattr_accessor.
thread_mattr_accessor uses class name as part of thread local variable name.

Following code set Thread[:attr_Account_user].

class Account thread_mattr_accessor :user end

But anonymous class set Thread[:attr_user]. Its subclass use same variable because
both of them don't have name.

If anonymous class use thread_mattr_accessor raise error by my pull request.
I will fix If it's needed.

If it worked before it would be nice to have it working still.

In this case I think it'd be better for it to raise: the previous behaviour treated all anonymous classes as one, so that doesn't seem terribly useful.

@senny OK. I added a commit not to raise error if use thread_mattr_accessor with anonymous class.

Oh, Bad timing. Which is do you think better? @senny

What @matthewd pointed out sounds reasonable. If it didn't work properly before then we should be good to raise. Would be nice to have the inline classes in the test-case work though. Maybe using def name; "MyClass" end does the trick.

I reverted a previous commit and add a commit using the trick to use anonymous class in test case 😃.

senny · 2016-07-25T15:28:59Z

@willnet good catch! Added a minor note about the test style. If possible I'd like to stick to the anonymous classes that we had before.

senny · 2016-08-02T08:34:10Z

@willnet can you squash your changes into a single commit? There's no need for us to keep track of that revert in your branch. Makes the history a lot cleaner if everything related to a change is in the same commit. Thanks.

willnet · 2016-08-02T13:00:14Z

@senny OK, I did it 😃.

senny · 2016-08-02T13:19:29Z

@willnet awesome! Sorry one last thing I forgot to mention, could you add an entry to the CHANGELOG outlining the fix?

willnet · 2016-08-02T15:05:07Z

@senny OK, I did it 😄.

The current implementation of `thread_mattr_accessor` set variable sharing superclass with subclass. So the method doesn't work as documented. Precondition class Account thread_mattr_accessor :user end class Customer < Account end Account.user = "DHH" Account.user #=> "DHH" Customer.user = "Rafael" Customer.user # => "Rafael" Documented behavior Account.user # => "DHH" Actual behavior Account.user # => "Rafael" Current implementation set variable statically likes `Thread[:attr_Account_user]`, and customer also use it. Make variable name dynamic to use own thread-local variable.

Fix `thread_mattr_accessor` share variable superclass with subclass

senny · 2016-08-08T10:39:53Z

@willnet thanks 💛 ! Merged with a slightly modified version of the CHANGELOG entry.

Fix `thread_mattr_accessor` share variable superclass with subclass

senny · 2016-08-08T10:56:14Z

Backported to 5-0-stable: 3469a1b

willnet · 2016-08-08T12:54:43Z

fxn · 2016-08-08T14:50:23Z

Let me double-check this patch, just in case.

Given:

class A
  thread_cattr_accessor :foo
  self.foo = 1
end

class B < A
end

before this change the attribute was shared/inherited:

B.foo # => 1

and after this patch the attribute is no longer shared/inherited:

B.foo # => nil

This interpolation has been there since the initial implementation.

Which documentation does the description of the PR refer to?

@dhh what do you think?

senny · 2016-08-08T14:57:51Z

@fxn here's the docs this was referring to: http://api.rubyonrails.org/classes/Module.html#method-i-thread_mattr_accessor

fxn · 2016-08-08T15:11:46Z

OK. And the sentence

if the parent class changes the value, the value of subclasses is not changed

also seems to match the new behaviour in my example.

Looking good then. The only thing that bothers me in these macros is that the documentation leaks the implementation (Thread.current[some_internal_key]. I believe end-users should just use the reader/writer interface.

senny · 2016-08-08T15:15:34Z

@fxn totally agree on leaking the implementation. I'm going to remove the Thread.current[:attr_Current_user] from the docs. I only looked at thread_mattr_accessor which interestingly doesn't show them. It's only in the reader / writer docs...

senny · 2016-08-08T15:39:28Z

@fxn on second thought, without the Thread.current references the docs for thread_mattr_reader and thread_mattr_writer don't make any sense. The thing is that these methods don't make a whole lot of sense without revealing the variable name. Why would you define a reader or a writer for a variable you don't know.

fxn · 2016-08-08T16:36:35Z

@senny totally agree.

A reader without a writer is useless if the user is not supposed to use the private key directly. And viceversa, why write something you cannot read? I think the public interface here is the accessor, and the reader/writer would be private methods that help implement the accessor.

If you want something to be private you can issue a manual private call.

This is a follow up to #25681, specifically this comment: #25681 (comment) The way the thread local variable is stored is an implementation detail and subject to change. It makes no sense to only generate a reader or writer as you'd have to know where to read from or where it writes to.

senny · 2016-10-20T19:23:52Z

I pushed a commit that hides the _reader and _writer methods from the API docs: dce4751

This is a follow up to #25681, specifically this comment: #25681 (comment) The way the thread local variable is stored is an implementation detail and subject to change. It makes no sense to only generate a reader or writer as you'd have to know where to read from or where it writes to.

atzorvas · 2017-05-19T20:24:44Z

activesupport/lib/active_support/core_ext/module/attribute_accessors_per_thread.rb

@@ -41,14 +41,14 @@ def thread_mattr_reader(*syms)
      raise NameError.new("invalid attribute name: #{sym}") unless /^[_A-Za-z]\w*$/.match?(sym)
      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
        def self.#{sym}
-          Thread.current[:"attr_#{name}_#{sym}"]
+          Thread.current["attr_"+ name + "_#{sym}"]


Why change the concat method? + is slower
https://github.com/JuanitoFatas/fast-ruby#string-concatenation-code

#{name} is different from + name + in class_eval.

#{name} is evaluated immediately on class_eval, but + name + is evaluated on executing method defined in class_eval

rails-bot assigned senny Jul 4, 2016

maclover7 added the activesupport label Jul 4, 2016

senny reviewed Jul 25, 2016
View reviewed changes

willnet force-pushed the fix-thread_mattr_accessor branch 2 times, most recently from 4eb6c98 to 345eaf5 Compare August 2, 2016 12:58

willnet force-pushed the fix-thread_mattr_accessor branch from 345eaf5 to 649cd96 Compare August 2, 2016 15:03

willnet force-pushed the fix-thread_mattr_accessor branch from 649cd96 to 3529e58 Compare August 4, 2016 01:11

senny merged commit 3529e58 into rails:master Aug 8, 2016

senny added a commit that referenced this pull request Aug 8, 2016

Merge pull request #25681 from willnet/fix-thread_mattr_accessor

e9852c9

Fix `thread_mattr_accessor` share variable superclass with subclass

senny added a commit that referenced this pull request Aug 8, 2016

Merge pull request #25681 from willnet/fix-thread_mattr_accessor

3469a1b

Fix `thread_mattr_accessor` share variable superclass with subclass

willnet deleted the fix-thread_mattr_accessor branch August 8, 2016 12:54

atzorvas reviewed May 19, 2017

View reviewed changes

ghiculescu mentioned this pull request Sep 16, 2021

thread_cattr_accessor behaves different from cattr_access for subclasses, even when not using threads. #43228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `thread_mattr_accessor` share variable superclass with subclass #25681

Fix `thread_mattr_accessor` share variable superclass with subclass #25681

willnet commented Jul 4, 2016

rails-bot commented Jul 4, 2016

senny Jul 25, 2016

willnet Jul 28, 2016

willnet Jul 28, 2016

senny Jul 28, 2016

matthewd Jul 30, 2016

willnet Jul 30, 2016

willnet Jul 30, 2016

senny Aug 1, 2016

willnet Aug 1, 2016

senny commented Jul 25, 2016

senny commented Aug 2, 2016

willnet commented Aug 2, 2016

senny commented Aug 2, 2016

willnet commented Aug 2, 2016

senny commented Aug 8, 2016

senny commented Aug 8, 2016

willnet commented Aug 8, 2016

fxn commented Aug 8, 2016

senny commented Aug 8, 2016

fxn commented Aug 8, 2016

senny commented Aug 8, 2016

senny commented Aug 8, 2016

fxn commented Aug 8, 2016 •

edited

senny commented Oct 20, 2016

atzorvas May 19, 2017 •

edited

willnet May 20, 2017

Fix thread_mattr_accessor share variable superclass with subclass #25681

Fix thread_mattr_accessor share variable superclass with subclass #25681

Conversation

willnet commented Jul 4, 2016

Summary

rails-bot commented Jul 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senny commented Jul 25, 2016

senny commented Aug 2, 2016

willnet commented Aug 2, 2016

senny commented Aug 2, 2016

willnet commented Aug 2, 2016

senny commented Aug 8, 2016

senny commented Aug 8, 2016

willnet commented Aug 8, 2016

fxn commented Aug 8, 2016

senny commented Aug 8, 2016

fxn commented Aug 8, 2016

senny commented Aug 8, 2016

senny commented Aug 8, 2016

fxn commented Aug 8, 2016 • edited

senny commented Oct 20, 2016

atzorvas May 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `thread_mattr_accessor` share variable superclass with subclass #25681

Fix `thread_mattr_accessor` share variable superclass with subclass #25681

fxn commented Aug 8, 2016 •

edited

atzorvas May 19, 2017 •

edited