stub_const doesn't clear Class#subclasses
#1568
Comments
|
It seems that doing a garbage collection after |
|
I tried on a Rails project and doing a garbage collection is not enough. Defined classes without |
|
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
|
|
For |
I mean,
Note that I see than everytime we use I understand that as As we see in this example, I don't know how to be sure the class than is removed. |
|
Defining the class in a global variable fix the issue : But I suppose it will create some other errors. |
|
@pirj can you confirm this is a bug or, at least, something that is supposed to be handled in rspec-mocks ? |
|
Something feels off, but the essence of the problem evades me so far. The 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 |
It doesn't surprise me that much. Every
The error is not actually raised when I do 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 |
|
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 Do you think RSpec is the one responsible? What is the original problem you are dealing with? Do you use |
|
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... |
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.
No, I don't.
Yes, I do. We have something like : I'm not sure if this is the best implementation but that's not the point. Rails overrides It's pseudo code, it doesn't work. I don't like to override |
|
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 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. |
Yes, that's it. The method name should probably be |
|
I succeeded to fix my issue with : I don't like to do override a Ruby method but see no other solution. I will create a PR with this code. |
|
As of ruby 3.1 and rails 7.0, This is an issue with ruby itself. See note attached to Class#subclasses My local reproducer running in 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
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 |
|
It's not an issue with Ruby. It's just how Ruby works. |
|
It was brought up by
|
|
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. |
|
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.
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. 🤔 |
|
Hey, this thread is long, let me dig into 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 If we did Let me also /cc @byroot in case he'd like to chime in (he wrote |
|
Yeah, I only skimmed the initial issue description, and this isn't a bug, it's how it's supposed to work.
And as long as class is alive, it will be reachable via it's parent I see how that may cause you issue, but it's not possible to create this illusion. |
|
@byroot So, on a Rails application, after N reloads, |
|
Ah, scratch that, we have the descendants tracker in place. |
|
@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 |
|
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. |
Ruby |
|
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 Also, in the general case, multiple serial calls to (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.) |
|
Let me also point out that this is documented. Not unlike |
|
Let me be more explicit with the consequences of my remarks above. I believe this issue is only tangentially related to
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 |
|
Hmmm, I don't know if it is clear, but From the second spec on, |
|
The crux of the issue from our perspective is "is it reasonable to expect us to clear our created classes from subclasses" or is this just something we should document as a "buyer beware" scenario, #1570 has been on my "to review" list for an embarassingly long time and I still have concerned about its implementation but as an opt in feature I think its a reasonable ask? IDK |
|
The way I see it, it is not reasonable. Take this reproduction that eliminates require 'bundler/inline'
gemfile(true) do
source 'https://rubygems.org'
gem 'rspec'
end
require 'rspec/autorun'
C = Class.new
describe 'Test' do
before(:each) { Class.new(C) }
it 'something' do
expect(C.subclasses.count).to eq(1)
end
it 'something else' do
expect(C.subclasses.count).to eq(1)
end
endThat does not pass, because one spec sees two subclasses, even if none of them are reachable. (Indeed, we could get counts of 0, 1, or 2). The problem with Subclasses "leak" from one spec to the next one, and RSpec cannot do anything there. Users are responsible for addressing that if they need subclasses to be reset between specs. |
|
I run the PR I did #1570 since a few months and have no problem with it. @fxn I'm not sure to understand the solution you propose.
You mean that we have to we have to change |
|
Ok I think I'm convinced that this is "behaving correctly", |
Subject of the issue
When
stub_constis 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
Steps to reproduce
Expected behavior
Something.subclassesalways have to return[B, A]and be the same for each spec.Actual behavior
Something.subclassesstill return every stubbed classes.The text was updated successfully, but these errors were encountered: