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

thread_mattr_accessor default value does not apply to threads #43312

Closed
tdeo opened this issue Sep 27, 2021 · 9 comments · Fixed by #43337
Closed

thread_mattr_accessor default value does not apply to threads #43312

tdeo opened this issue Sep 27, 2021 · 9 comments · Fixed by #43337

Comments

@tdeo
Copy link
Contributor

tdeo commented Sep 27, 2021

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support/core_ext/module/attribute_accessors_per_thread"
require "minitest/autorun"

class BugTest < Minitest::Test
  class ThreadAccessors
    thread_mattr_accessor :variable, default: 42
  end

  def test_stuff
    assert(ThreadAccessors.variable == 42)
    Thread.new do
      assert(ThreadAccessors.variable == 42)
    end.join
  end
end

Expected behavior

I'd expect the default value for the thread variable to be present in all threads

Actual behavior

The variable is initialized only in the main thread at load time and nil in other threads

System configuration

Rails version: main branch

Ruby version: ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]

@intrip
Copy link
Contributor

intrip commented Sep 27, 2021

I think this is the expected behaviour: main thread != a new thread.
Under the hood thread_mattr_accessor relies on Thread.current[:name] please check the related docs to find more info.

@tdeo
Copy link
Contributor Author

tdeo commented Sep 27, 2021

What I mean is that I find misleading that thread_mattr_accessor supports a default option which is not always applied. When I define a class having a thread_mattr_accessor :variable, default: 0, I would expect calling Class.variable to never be nil (unless I set it manually myself), but always have its default value of 0 before an explicit assignment.

My example makes very explicit what's happening with Thread.new, however, in real life, you could end up in that situation because your code is being called from a threaded production environment (be it a job framework or a web server), and it's quite opaque to the end-user.

@intrip
Copy link
Contributor

intrip commented Sep 28, 2021

@tdeo Ah I see your point now!

I agree that this behaviour is counter intuitive, on the other hand changing it introduces a backward incompatibility.

@ghiculescu what's your take on this?

@ghiculescu
Copy link
Member

ghiculescu commented Sep 28, 2021

Thanks for the issue and replication test. There was a recent issue similar to this: #43228

It's expected + documented behaviour, but I can accept it's also confusing. I think https://discuss.rubyonrails.org/c/rubyonrails-core/5 is the right venue to discuss it, since it is behaving as documented. @tdeo would you like to start a thread there? (it doesn't look like anyone did since the last issue)

@tdeo
Copy link
Contributor Author

tdeo commented Sep 28, 2021

I'm not talking about the same issue (that previous one deals with default value in subclasses, and subclasses behavior is somewhat documented).

However, I can't find any explanation for the expected default option behavior in the doc for thread_mattr_accessor, is it somewhere else?

@ghiculescu ghiculescu reopened this Sep 28, 2021
@ghiculescu
Copy link
Member

Oooops, sorry, I read that too quickly.

@ghiculescu
Copy link
Member

Chatted to @matthewd and he agreed this is probably a bug. A PR would be welcome, with the caveat that it not kill performance in the reader (but that could be a nontrivial caveat).

@tdeo
Copy link
Contributor Author

tdeo commented Sep 29, 2021

Hello @ghiculescu,

From the top of my mind, here is the patched version I would come up with:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "benchmark-ips"
end

require "active_support"
require "active_support/core_ext/module/attribute_accessors_per_thread"
require "minitest/autorun"

# Your patch goes here.
class Module
  def thread_mattr_reader_default(*syms, instance_reader: true, instance_accessor: true, default: nil)
    syms.each do |sym|
      raise NameError.new("invalid attribute name: #{sym}") unless /^[_A-Za-z]\w*$/.match?(sym)
      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
        def self.#{sym}
          if Thread.current["attr_defined_" + name + "_#{sym}"].nil?
            Thread.current["attr_defined_" + name + "_#{sym}"] = true
            Thread.current["attr_" + name + "_#{sym}"] = #{default.inspect}
          end
          Thread.current["attr_" + name + "_#{sym}"]
        end
      EOS

      if instance_reader && instance_accessor
        class_eval(<<-EOS, __FILE__, __LINE__ + 1)
          def #{sym}
            self.class.#{sym}
          end
        EOS
      end
    end
  end

  def thread_mattr_writer_default(*syms, instance_writer: true, instance_accessor: true, default: nil) # :nodoc:
    syms.each do |sym|
      raise NameError.new("invalid attribute name: #{sym}") unless /^[_A-Za-z]\w*$/.match?(sym)

      # The following generated method concatenates `name` because we want it
      # to work with inheritance via polymorphism.
      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
        def self.#{sym}=(obj)
          if Thread.current["attr_defined_" + name + "_#{sym}"].nil?
            Thread.current["attr_defined_" + name + "_#{sym}"] = true
          end
          Thread.current["attr_" + name + "_#{sym}"] = obj
        end
      EOS

      if instance_writer && instance_accessor
        class_eval(<<-EOS, __FILE__, __LINE__ + 1)
          def #{sym}=(obj)
            self.class.#{sym} = obj
          end
        EOS
      end

      public_send("#{sym}=", default) unless default.nil?
    end
  end

  def thread_mattr_accessor_default(*syms, instance_reader: true, instance_writer: true, instance_accessor: true, default: nil)
    thread_mattr_reader_default(*syms, instance_reader: instance_reader, instance_accessor: instance_accessor, default: default)
    thread_mattr_writer_default(*syms, instance_writer: instance_writer, instance_accessor: instance_accessor)
  end
end

module A
  thread_mattr_accessor_default :attribute_with_default, default: 42
  thread_mattr_accessor :attribute, default: 42
end

Benchmark.ips do |x|
  x.report("original")      { A.attribute }
  x.report("patched_default") { A.attribute_with_default }
  x.compare!
end

class BugTest < Minitest::Test
  def test_stuff
    assert(A.attribute_with_default == 42)
    Thread.new do
      assert(A.attribute_with_default == 42)
    end.join
    A.attribute_with_default = nil
    assert(A.attribute_with_default.nil?)
  end
end

Output of this script:

Warming up --------------------------------------
            original   170.500k i/100ms
     patched_default    66.095k i/100ms
Calculating -------------------------------------
            original      2.105M (± 5.4%) i/s -     10.571M in   5.036874s
     patched_default    914.367k (± 7.5%) i/s -      4.561M in   5.018758s

Comparison:
            original:  2104980.9 i/s
     patched_default:   914367.3 i/s - 2.30x  (± 0.00) slower

Run options: --seed 65131

# Running:

.

Finished in 0.002085s, 479.6453 runs/s, 1438.9359 assertions/s.
1 runs, 3 assertions, 0 failures, 0 errors, 0 skips

The performance hit is indeed pretty nasty (x2.3), however, that's still ~1M ips, which I'm not sure if that would be acceptable for such a feature, let me know what you think.

Unfortunately, I can't think of a way to do it without a second thread variable as the following occurs when explicitly setting it to nil:

Thread.current[:var] = 1
Thread.current.key?(:var) # => true 
Thread.current[:var] = nil
Thread.current.key?(:var) # => false 

@jonathanhefner
Copy link
Member

jonathanhefner commented Sep 29, 2021

@tdeo

the following occurs when explicitly setting it to nil:

Thread.current[:var] = 1
Thread.current.key?(:var) # => true
Thread.current[:var] = nil
Thread.current.key?(:var) # => false

That's unfortunate. 🤔 What about using a placeholder object for nil? Something like:

    NIL_OBJECT = Object.new

    # ...

      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
        def self.#{sym}
          key = "attr_" + name + "_#{sym}"
          obj = Thread.current[key]
          
          if obj.nil?
            obj = #{default.inspect}
            Thread.current[key] = obj.nil? ? NIL_OBJECT : obj
          end

          NIL_OBJECT == obj ? nil : obj
        end
      EOS

    # ...

      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
        def self.#{sym}=(obj)
          Thread.current["attr_" + name + "_#{sym}"] = obj.nil? ? NIL_OBJECT : obj
        end
      EOS

    # ...

That does keep a thread variable allocated, even when the value (or default) is nil, but perhaps that's not so bad. (Related to performance: it might save a few string allocations to change "attr_" + name + "_#{sym}" to "attr_\#{name}_#{sym}".)

tdeo added a commit to tdeo/rails that referenced this issue Apr 12, 2022
This makes the value supplier to the `default` option of
`thread_mattr_accessor` to be set in descendant classes
as well as in any new Thread that starts.

Previously, the `default` value provided was set only at the
moment of defining the attribute writer, which would cause
the attribute to be uninitialized in descendants and in other threads.

For instance:

    class Processor
      thread_mattr_accessor :mode, default: :smart
    end

    class SubProcessor < Processor
    end

    SubProcessor.mode # => :smart
    Thread.new do
      Processor.mode # => :smart
    end.join

when in the past, those two calls would have returned `nil`.

This logic comes with a performance impact on the reader,
here is the benchmark:

    Warming up --------------------------------------
            original   238.780k i/100ms
     patched_default   162.765k i/100ms
    Calculating -------------------------------------
            original    2.376M (± 9.4%) i/s -  11.939M in 5.078355s
     patched_default    1.635M (±16.4%) i/s -   7.813M in 5.008433s

    Comparison:
            original:  2375953.1 i/s
     patched_default:  1634668.2 i/s - 1.45x  (± 0.00) slower

Fixes rails#43312.
jonathanhefner added a commit to tdeo/rails that referenced this issue Aug 25, 2022
This makes the value supplied to the `default` option of
`thread_mattr_accessor` to be set in descendant classes
as well as in any new Thread that starts.

Previously, the `default` value provided was set only at the
moment of defining the attribute writer, which would cause
the attribute to be uninitialized in descendants and in other threads.

For instance:

  ```ruby
  class Processor
    thread_mattr_accessor :mode, default: :smart
  end

  class SubProcessor < Processor
  end

  SubProcessor.mode # => :smart (was `nil` prior to this commit)

  Thread.new do
    Processor.mode # => :smart (was `nil` prior to this commit)
  end.join
  ```

If a non-`nil` default has been specified, there is a small (~7%)
performance decrease when reading non-`nil` values, and a larger (~45%)
performance decrease when reading `nil` values.

Benchmark script:

  ```ruby
  # frozen_string_literal: true
  require "benchmark/ips"
  require "active_support"
  require "active_support/core_ext/module/attribute_accessors_per_thread"

  class MyClass
    thread_mattr_accessor :default_value, default: "default"
    thread_mattr_accessor :string_value, default: "default"
    thread_mattr_accessor :nil_value, default: "default"
  end

  MyClass.string_value = "string"
  MyClass.nil_value = nil

  Benchmark.ips do |x|
    x.report("default_value") { MyClass.default_value }
    x.report("string_value") { MyClass.string_value }
    x.report("nil_value") { MyClass.nil_value }
  end
  ```

Before this commit:

  ```
         default_value      2.075M (± 0.7%) i/s -     10.396M in   5.010585s
          string_value      2.103M (± 0.7%) i/s -     10.672M in   5.074624s
             nil_value      1.777M (± 0.9%) i/s -      8.924M in   5.023058s
  ```

After this commit:

  ```
         default_value      2.008M (± 0.7%) i/s -     10.187M in   5.072990s
          string_value      1.967M (± 0.7%) i/s -      9.891M in   5.028570s
             nil_value      1.144M (± 0.5%) i/s -      5.770M in   5.041630s
  ```

If no default or a `nil` default is specified, there is no performance
impact.

Fixes rails#43312.

Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
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.

4 participants