`stub_const` doesn't clear `Class#subclasses`

[GCorbel]

Subject of the issue

When stub_const is used with a class that is a subclass of another, the class is still listed in subclasses of this parent after each spec.

Your environment

• Ruby version: 3.1.4
• rspec-mocks version: 3.12.6

Steps to reproduce

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

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

  gem "rspec", "3.12.0" # Activate the gem and version you are reporting the issue against.
  gem 'rspec-mocks', '3.12.6'
end

puts "Ruby version is: #{RUBY_VERSION}" # Ruby version is: 3.1.4

class Something; end

describe 'Test' do
  before(:each) do
    class A < Something; end
    stub_const('B', Class.new(Something))
  end

  it 'something' do
    puts Something.subclasses # => [B, A]
  end

  it 'something else' do
    puts Something.subclasses # => [B, B, A]
    # Only one occurence of B should be listed
  end
end

Expected behavior

Something.subclasses always have to return [B, A] and be the same for each spec.

Actual behavior

Something.subclasses still return every stubbed classes.

[GCorbel]

It seems that doing a garbage collection after RSpec::Mocks.teardown fix the issue.

class Something; end

describe 'Test' do
  before(:each) do
    class A < Something; end
    stub_const('B', Class.new(Something))
  end

  after(:each) do
    RSpec::Mocks.teardown
    GC.start
  end

  it 'something' do
    pp Something.subclasses # => [B, A]
  end

  it 'something else' do
    pp Something.subclasses # => [B, A]
  end
end

[GCorbel]

I tried on a Rails project and doing a garbage collection is not enough. Defined classes without stub_const are correctly removed and those that use stub_const aren't.

[pirj]

What do you mean "correctly removed"?

describe 'Test', order: :defined do
  context 'with A' do
    before(:each) do
      class A < Something; end
    end

    it 'something' do
      pp Something.subclasses # => [A]
    end
  end

  context 'presumably without A' do
    it 'something else' do
      pp Something.subclasses # => [A] 💥
    end
  end
end

A won't go anywhere.

[pirj]

For stub_const, wondering, what the @parent is, and why calling remove_const on it doesn't reset its subclasses?

[GCorbel]

What do you mean "correctly removed"?

I mean, Something.subclasses have to be the same for each spec. I edited the ticket's description.

RSpec::Mocks.space.instance_variable_get(:@constant_mutators) gives :

[#<RSpec::Mocks::ConstantMutator::UndefinedConstantSetter:0x00007fb2c9e84c10
  @const_name="B",
  @context_parts=[],
  @full_constant_name="B",
  @mutated_value=B,
  @parent=Object,
  @reset_performed=false,
  @transfer_nested_constants=nil>]

Object.send(:remove_const, :B) gives :

     NameError:
       constant Object::B not defined
     
                 @parent.__send__(:remove_const, @const_name)

Note that B.new raise no error.

I see than everytime we use Class.new(Something), it is added in subclasses of Something but not with class A < Something; end :

    class A < Something; end
    class A < Something; end
    class A < Something; end
    class A < Something; end
    class A < Something; end
    stub_const('B', Class.new(Something))
    stub_const('B', Class.new(Something))
    stub_const('B', Class.new(Something))
    stub_const('B', Class.new(Something))
    stub_const('B', Class.new(Something))
    puts Something.subclasses.inspect 
    # => [B, B, B, B, B, A]

   puts ObjectSpace.each_object(Class).select { |klass| klass.name == 'A' }.count
   # => 1

   puts ObjectSpace.each_object(Class).select { |klass| klass.name == 'B' }.count
   # => 5

I understand that as Class.new is a completly different Class, with a different object_id while A gives the same object_id each time.

As we see in this example, B is still in ObjectSpace.

describe 'Test' do
  before(:each) do
    class A < Something; end
    stub_const('B', Class.new(Something))
  end

  after(:each) do
    pp ObjectSpace.each_object(Class).select { |klass| klass.name == 'A' }.count
    # Gives 1 twice as supposed
    pp ObjectSpace.each_object(Class).select { |klass| klass.name == 'B' }.count
    # Gives 1 the first time but 2 the second time
  end

  it 'something' do
  end

  it 'something else' do
  end
end

I don't know how to be sure the class than is removed.

[GCorbel]

Defining the class in a global variable fix the issue :

class Something; end

describe 'Test' do
  before(:each) do
    class A < Something; end
    $b ||= Class.new(Something)
    stub_const('B', $b)
  end

  after(:each) do
    pp Something.subclasses
    # => Gives [B, A] twice
  end

  it 'something' do
  end

  it 'something else' do
  end
end

But I suppose it will create some other errors.

[GCorbel]

@pirj can you confirm this is a bug or, at least, something that is supposed to be handled in rspec-mocks ?

[pirj]

Something feels off, but the essence of the problem evades me so far. The class A is not the correct contrast. Global var is closer.

What we know is that unreferenced classes are garbage-collected. But why subclasses show that there are two bound to consts with identical names?

I’m still not convienced that our observation about @parent is correct. It doesn’t blow up in your test? Why it does blow up if you run it manually?

[GCorbel]

But why subclasses show that there are two bound to consts with identical names?

It doesn't surprise me that much. Every Class.new is referenced as a completely different class so every time we do Object.send(:const_set, 'Bla', Class.new), another class is added in the ObjectSpace with the same name

I’m still not convienced that our observation about @parent is correct. It doesn’t blow up in your test? Why it does blow up if you run it manually?

The error is not actually raised when I do Object.send(:remove_const, :B) but after. Here is the full trace :

     Failure/Error: @parent.__send__(:remove_const, @const_name)
     
     NameError:
       constant Object::B not defined
     
                 @parent.__send__(:remove_const, @const_name)
                        ^^^^^^^^^
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:300:in `remove_const'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:300:in `reset'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:161:in `idempotently_reset'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `block in reset_all'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `each'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `reset_all'
     # /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks.rb:52:in `teardown'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/mocking_adapters/rspec.rb:27:in `teardown_mocks_for_rspec'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:521:in `run_after_example'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:283:in `block in run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `block in with_around_and_singleton_context_hooks'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `block in with_around_example_hooks'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `block in run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `with_around_example_hooks'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `with_around_and_singleton_context_hooks'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:259:in `run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:646:in `block in run_examples'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `map'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `run_examples'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:607:in `run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `map'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/configuration.rb:2070:in `with_suite_hooks'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:116:in `block in run_specs'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/reporter.rb:74:in `report'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:115:in `run_specs'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:89:in `run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:71:in `run'
     # /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:45:in `invoke'
     # /usr/local/bundle/gems/rspec-core-3.12.2/exe/rspec:4:in `<top (required)>'
     # /usr/local/bundle/bin/rspec:25:in `load'
     # /usr/local/bundle/bin/rspec:25:in `<top (required)>'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `load'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `kernel_load'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:23:in `run'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:484:in `exec'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:31:in `dispatch'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:25:in `start'
     # /usr/local/bundle/gems/bundler-2.3.4/exe/bundle:48:in `block in <top (required)>'
     # /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
     # /usr/local/bundle/gems/bundler-2.3.4/exe/bundle:36:in `<top (required)>'
     # /usr/local/bundle/bin/bundle:25:in `load'
     # /usr/local/bundle/bin/bundle:25:in `<main>'
     # 
     #   Showing full backtrace because every line was filtered out.
     #   See docs for RSpec::Configuration#backtrace_exclusion_patterns and
     #   RSpec::Configuration#backtrace_inclusion_patterns for more information.

I did those tests (none fix the issue)

  # This does not raise an error
  after(:each) do
    RSpec::Mocks.space.reset_all
  end

  # This does not raise an error
  after(:each) do
    RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
      mutator.idempotently_reset
    end
  end

  # This raise constant Object::B not defined
  after(:each) do
    Object.send(:remove_const, :B)
  end

  # This raise constant Object::B not defined
  after(:each) do
    RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
      mutator.instance_variable_get(:@parent).send(:remove_const, mutator.instance_variable_get(:@const_name))
    end
  end

  # This raise constant Object::B not defined
  after(:each) do
    RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
      mutator.reset
    end
  end

[pirj]

Sorry, I never meant you should call remove_const from the spec code. And if I did, I was meaning something else.

So we’re now at a point where we see that tge GC is involved, and yet-to-be-collected classes still appear through subclasses even though are not anymore referenced or even accessible.

Do you think RSpec is the one responsible?

What is the original problem you are dealing with? Do you use subclasses in your production code? In our own spec suite using various means for better isolation between examples, even running another process of RSpec, forking etc.

[JonRowe]

I do think this is a bug but globals is not the answer, if the issue is Ruby is retaining the class after we have removed it, I'm not sure of the solution to that...

[GCorbel]

So we’re now at a point where we see that tge GC is involved, and yet-to-be-collected classes still appear through subclasses even though are not anymore referenced or even accessible.

Relying on garbage collection can be tricky as the class can be kept in memory elsewhere, and I'm pretty sure it is with Rails.

Do you think RSpec is the one responsible?

No, I don't.

What is the original problem you are dealing with? Do you use subclasses in your production code?

Yes, I do. We have something like :

class Content
  def self.creatable_classes
    self.subclasses
  end

  def self.creatable_classes_for(user)
    creatable_classes.select { |klass| klass.creatable_for?(user) }
  end
end

class Article < Content
  def self.creatable_for?(user)
    user.admin?
  end
end

class Something < Content
  def self.creatable_for?(user)
    true
  end
end

Content.creatable_classes_for(user) 

# => [Article, Something] if the user is an admin and [Something] for others

I'm not sure if this is the best implementation but that's not the point.

Rails overrides subclasses when RubyFeatures::CLASS_SUBCLASSES is false. Maybe we can do something as :

module ExcludeClassesFromSubclasses
  @@stubbed_classes = []

  def self.stub_class(klass)
    @@stubbed_classes += klass
  end

  def self.subclasses
    super - @@stubbed_classes
  end
end

Class.prepend(ExcludeClassesFromSubclasses)

It's pseudo code, it doesn't work. I don't like to override Class but I suppose it will work.

[pirj]

That means that we’ll keep references to stubbed classes in between examples, effectively preventing from GC’ing them.

Also, the classes stubbed in an example should be returned by subclasses while the example runs, right? We should push them to the exclude list after teardown.

So, we’ll need an extra step and WeakRef.

Even though this would fix your case, I have no certainty we should include this for everyone and what the performance penalty is.

[GCorbel]

Also, the classes stubbed in an example should be returned by subclasses while the example runs, right? We should push them to the exclude list after teardown.

Yes, that's it. The method name should probably be hide or something like that.

[GCorbel]

I succeeded to fix my issue with :

diff --git a/lib/rspec/mocks.rb b/lib/rspec/mocks.rb
index 297779e5..b2beb79c 100644
--- a/lib/rspec/mocks.rb
+++ b/lib/rspec/mocks.rb
@@ -101,6 +101,23 @@ module RSpec
       end
     end
 
+    @@excluded_subclasses = []
+
+    def self.excluded_subclasses
+      @@excluded_subclasses.select(&:weakref_alive?).map(&:__getobj__)
+    end
+
+    def self.exclude_subclass(constant)
+      @@excluded_subclasses << WeakRef.new(constant)
+    end
+
+    module ExcludeClassesFromSubclasses
+      def subclasses
+        super - RSpec::Mocks.excluded_subclasses
+      end
+    end
+    Class.prepend(ExcludeClassesFromSubclasses)
+
     class << self
       # @private
       attr_reader :space
diff --git a/lib/rspec/mocks/mutate_const.rb b/lib/rspec/mocks/mutate_const.rb
index 071d7a21..d702c79d 100644
--- a/lib/rspec/mocks/mutate_const.rb
+++ b/lib/rspec/mocks/mutate_const.rb
@@ -297,6 +297,7 @@ module RSpec
         end
 
         def reset
+          RSpec::Mocks.exclude_subclass(@mutated_value)
           @parent.__send__(:remove_const, @const_name)
         end
 

I don't like to do override a Ruby method but see no other solution. I will create a PR with this code.

[kbrock]

As of ruby 3.1 and rails 7.0, DescendantsTracker#subclasses is basically stubbed out ref.

This is an issue with ruby itself. See note attached to Class#subclasses

My local reproducer running in irb to show it is not an rspec or rails issue.

class A ; end                  # => nil
class B < A ; end              #=> nil
Object.send(:remove_const, :B) #=> B
B                              # uninitialized constant B (NameError)
A.subclasses.first             # => B
# sometimes this works, other times it does not.
# Haven't nailed down the exact voodoo here
3.times { GC.start }
sleep(1)
3.times { GC.start }
A.subclasses.first             # => nil

stub_constant is just a simple wrapper. I think of it as

before { Object.const_add(:B, value) }
after  { Object.send(:remove_const, :B }

So if the underlying infrastructure is not working, then I'm not sure how rspec can fix that.

The code proposed above sure feel like code in rail's DescendantTracker.

[pirj]

It's not an issue with Ruby. It's just how Ruby works. See https://github.com/rspec/rspec-mocks/pull/1570 for more detail.

[Fryguy]

It was brought up by @fxn early on in https://bugs.ruby-lang.org/issues/18273 and even in the docs discussion https://github.com/ruby/ruby/pull/5480, but it seemed to not be important enough to have .subclasses be accurate, and unfortunately this is the side effect of that.

GC.start does remove it however (even after a single GC for me on ruby 3.1, 3.2 and 3.3). I think the fix might be as simple as adding a GC.start immediately after the remove_const call? Something like: https://github.com/Fryguy/rspec-mocks/commit/c480918a8e72b8376480887c7d47df628f61ea98 ?

[pirj]

Having to run GC.start after each example that had stub_const is wasteful.

This could be done automatically somehow, with e.g patching stub_const to set a flag that a GC is needed to prevent a leak, and an after hook that would run that GC.

But this won’t work if classes are inherited and assigned a constant in the code being tested. It would only work with Class.new(Parent) and stub_const.

Another approach is to patch ‘subclasses’ as in the linked PR until the GC happens naturally.

I would prefer this to be fixed in Ruby itself, but so far halfway through reading the discussion on ruby-lang you referred to, I don’t think there’s a consensus, even though I side with fxn on the practical application.

[Fryguy]

I'm curious what @fxn thinks here. I was avoiding pinging him, but in my case zeitwerk is involved here, and stub_const + zeitwerk + remove_const not working as expected is an interesting combination here.

Another approach is to patch ‘subclasses’ as in the linked PR until the GC happens naturally.

The "patch 'subclasses'" made me think back the pre-ruby-3.1 Rails DescendantsTracker and its approach since that's exactly what it was doing (i.e. patching the subclasses method). I understand 3.1+ subclasses is more performant, but it's also inaccurate. I kind of wish we could just resurrect the DescendantsTracker code here. 🤔

[fxn]

Hey, this thread is long, let me dig into stub_const and the rest of the conversation later.

Just in case it helps, the root issue may be that classes now have a weakref to their subclasses (as @Fryguy says), but being a weakref does not seem to be enough:

GC.disable

class C
end

class D < C
end

p C.subclasses # => [D]
Object.send(:remove_const, :D)
p C.subclasses # => [D]

From the point of view of the Ruby programmer, the object stored in D is elegible for GC and therefore would expect it to be gone in the public interface. Is that a leak? (In a conceptual sense, not memory.)

If we did d = D.new and after const removal we ask for d.class, that would be different, it is expected to be there with a strong reference (therefore not GC'ed).

Let me also /cc @byroot in case he'd like to chime in (he wrote subclasses).

[byroot]

Yeah, I only skimmed the initial issue description, and this isn't a bug, it's how it's supposed to work.

stub_const has to restore the original value, hence it can't let ot be garbage collected, it has to keep holding onto it.

And as long as class is alive, it will be reachable via it's parent #subclasses method.

I see how that may cause you issue, but it's not possible to create this illusion.

[fxn]

@byroot So, on a Rails application, after N reloads, ActiveRecord::Base.subclasses could theoretically be returning N + 1 class objects for every ApplicationRecord being reloaded?

[fxn]

Ah, scratch that, we have the descendants tracker in place.

[Fryguy]

@byroot that's a great point that the code must have a strong reference to the original constant in order to restore it. I'm not sure how we get around that without some hiding mechanism that is honored by .subclasses.

[Fryguy]

Kind of a tangent, but one thing I do find strange is that when calling .subclasses after remove_const it presents the class as if the constant still exists (e.g. [D]). I'm sure it's something about how it presents the class name, but I was expecting the output to look like an anonymous class (e.g. [#<Class:00012345678>]). Almost feels like remove_const should also remove the name attribute in the class somehow.

[byroot]

I'm sure it's something about how it presents the class name, but I was expecting the output to look like an anonymous class (e.g. [#<Class:00012345678>])

Ruby Module instances get their permanent name the first time they are assigned to a constant. This is normal and expected.

[fxn]

This is the minimal reproduction, I think:

class C; end
Class.new(C)
p C.subclasses # => [#<Class:0x0000000102743c10>]

The subclass was not even stored anywhere.

It is all ramifications of Class#subclasses holding a weakref.

Also, in the general case, multiple serial calls to Class#subclasses in a single-threaded program can return different results. Because they depend on GC.

(Just following up for the sake of curiosity, in this case there is a strong reference. Point is, even without a strong reference, you could not guarantee the wanted behavior.)

[fxn]

Let me also point out that this is documented.

Not unlike ObjectSpace. Time ago I looked for a way to detect stale objects after a reload by inspecting ObjectSpace, but unreachable objects may be there, and there is no reachable? that I am aware of. So I don't know if that is possible. Not even GC.start guarantees a cleanup.

[fxn]

Let me be more explicit with the consequences of my remarks above.

I believe this issue is only tangentially related to stub_const and essentially unrelated to the fact that there is a strong reference anywhere. It goes like this:

1. The :each hook is creating subclasses of Something.
2. Class#subclasses depends on GC.
3. Therefore, you cannot expect a reset of Class#subclasses between specs.
4. Therefore, those tests are not reliable, even if stub_const was not involved.

I believe this issue can be closed, this is out of the control of RSpec, it is just Ruby.

If the application needs that behavior, they need to take control over Class#subclasses and have API to reset manually.