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

Fix `thread_mattr_accessor` share variable superclass with subclass #25681

Merged
merged 1 commit into from Aug 8, 2016

Conversation

Projects
None yet
7 participants
@willnet
Contributor

willnet commented Jul 4, 2016

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

This comment has been minimized.

rails-bot commented Jul 4, 2016

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

View changes

activesupport/test/core_ext/module/attribute_accessor_per_thread_test.rb Outdated
thread_mattr_reader :shaq, instance_reader: false
thread_mattr_accessor :camp, instance_accessor: false
end
class Foo

This comment has been minimized.

@senny

senny Jul 25, 2016

Member

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

@class = Class.new do

end

@subclass = Class.new(@class) do

end

not work?

This comment has been minimized.

@willnet

willnet Jul 28, 2016

Contributor

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.

This comment has been minimized.

@willnet

willnet Jul 28, 2016

Contributor

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

This comment has been minimized.

@senny

senny Jul 28, 2016

Member

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

This comment has been minimized.

@matthewd

matthewd Jul 30, 2016

Member

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.

This comment has been minimized.

@willnet

willnet Jul 30, 2016

Contributor

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

This comment has been minimized.

@willnet

willnet Jul 30, 2016

Contributor

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

This comment has been minimized.

@senny

senny Aug 1, 2016

Member

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.

This comment has been minimized.

@willnet

willnet Aug 1, 2016

Contributor

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

@senny

This comment has been minimized.

Member

senny commented Jul 25, 2016

@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

This comment has been minimized.

Member

senny commented Aug 2, 2016

@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 willnet force-pushed the willnet:fix-thread_mattr_accessor branch 2 times, most recently Aug 2, 2016

@willnet

This comment has been minimized.

Contributor

willnet commented Aug 2, 2016

@senny OK, I did it 😃.

@senny

This comment has been minimized.

Member

senny commented Aug 2, 2016

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

@willnet willnet force-pushed the willnet:fix-thread_mattr_accessor branch Aug 2, 2016

@willnet

This comment has been minimized.

Contributor

willnet commented Aug 2, 2016

@senny OK, I did it 😄.

Fix `thread_mattr_accessor` share variable superclass with subclass
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.

@willnet willnet force-pushed the willnet:fix-thread_mattr_accessor branch to 3529e58 Aug 4, 2016

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

2 checks passed

codeclimate Code Climate found 4 fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request #25681 from willnet/fix-thread_mattr_accessor
Fix `thread_mattr_accessor` share variable superclass with subclass
@senny

This comment has been minimized.

Member

senny commented Aug 8, 2016

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

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

Merge pull request #25681 from willnet/fix-thread_mattr_accessor
Fix `thread_mattr_accessor` share variable superclass with subclass
@senny

This comment has been minimized.

Member

senny commented Aug 8, 2016

Backported to 5-0-stable: 3469a1b

@willnet

This comment has been minimized.

Contributor

willnet commented Aug 8, 2016

寿司ゆき:thanks

@willnet willnet deleted the willnet:fix-thread_mattr_accessor branch Aug 8, 2016

@fxn

This comment has been minimized.

Member

fxn commented Aug 8, 2016

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

This comment has been minimized.

Member

senny commented Aug 8, 2016

@fxn

This comment has been minimized.

Member

fxn commented Aug 8, 2016

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

This comment has been minimized.

Member

senny commented Aug 8, 2016

@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

This comment has been minimized.

Member

senny commented Aug 8, 2016

@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

This comment has been minimized.

Member

fxn commented Aug 8, 2016

@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.

senny added a commit that referenced this pull request Oct 20, 2016

doc, hide non-public methods form the api docs. [ci skip]
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

This comment has been minimized.

Member

senny commented Oct 20, 2016

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

senny added a commit that referenced this pull request Oct 20, 2016

doc, hide non-public methods form the api docs. [ci skip]
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.
@@ -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}"]

This comment has been minimized.

@atzorvas

This comment has been minimized.

@willnet

willnet May 20, 2017

Contributor

#{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

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