New threadsafe let implementation can cause NoMethodError when closures retain access to the example

[myronmarston]

I updated a work project to HEAD and immediately ran into a bunch of NoMethodError:

     NoMethodError:
       undefined method `fetch_or_store' for nil:NilClass

I traced it down to a poorly written spec of mine that did the equivalent of:

let(:redis) { logic_to_get_redis_instance }
before do
  MyJobPipeline.configure do |c|
    c.define_redis_instance_for { |key| redis }
  end
end

Calling define_redis_instance_for mutates MyJobPipeline global state. I should be resetting it back after the examples but forgot to. The define_redis_instance_for blocks retains a reference to the example, and calls redis on it each time MyJobPipeline invokes the configured block.

Before #1858 was merged, this poorly written spec didn't cause problems, since __memoized was deifned as @__memoized ||= {} -- so if the variable was nil, it would merely get re-initialized to a blank hash. As of #1858, we no longer have the ||= {} logic -- instead @__memoized is eagerly initialized. After the example completes, all instance variables are set to nil (see b51be2c which addressed #321), so the when the let method is invoked after the example has completed, it raises a NoMethodError.

Ultimately, my spec is buggy and should be fixed (and I will) but the experience of RSpec raising a NoMethodError in this case is poor and I think we should address that, too. If I can accidentally forget to reset the global state like this, users can, too.

The question is...how do we address this w/o bringing back the memory leak of #321?

I'm thinking that maybe we can remove the clearing of the ivars entirely, and instead figure out what's retaining a reference to the example group instance and clear that instead. That should be faster, anyway as it's only 1 thing to clear instead of N things (for N ivars).

Any reason that wouldn't work? @dchelimsky, when fixing #321, did you consider the route of clearing the reference to the example group instance rather than clearing the instance's references to its ivars? (If you don't remember, as would be totally reasonable, that's fine, but figured I'd ask).

[JonRowe]

We should definitely fix this before shipping the next release :) can we somehow restore the original behaviour or was that merely masking this bug?

[myronmarston]

I'm planning to fix this before next release -- that's why it's labeled "release blocker" :).

[JoshCheek]

Any reason that wouldn't work? @dchelimsky, when fixing #321, did you consider the route of clearing the reference to the example group instance rather than clearing the instance's references to its ivars? (If you don't remember, as would be totally reasonable, that's fine, but figured I'd ask).

Looks like it does both, but my understanding is questionable.

[JoshCheek]

Also, my suite passes when I delete that code:

# I deleted the code that resets ivars
$ git diff
diff --git a/lib/rspec/core/example.rb b/lib/rspec/core/example.rb
index c38a260..e7685ae 100644
--- a/lib/rspec/core/example.rb
+++ b/lib/rspec/core/example.rb
@@ -225,9 +225,6 @@ module RSpec
         rescue Exception => e
           set_exception(e)
         ensure
-          ExampleGroup.each_instance_variable_for_example(@example_group_instance) do |ivar|
-            @example_group_instance.instance_variable_set(ivar, nil)
-          end
           @example_group_instance = nil
         end


# The entire suite passes
$ ruby --disable-gem -S bundle/bin/rspec
# ...
Finished in 7.65 seconds (files took 0.73697 seconds to load)
1696 examples, 0 failures, 1 pending


# I'm on master as of this commit 3 days ago https://github.com/rspec/rspec-core/commit/35106b316e589d12b1661e3478d2b8210ad06693
$ git log --pretty=format:'%H' -1
35106b316e589d12b1661e3478d2b8210ad06693


# Add a spec that illustrates the issue
$ cat <<SPEC > spec/zomg_spec.rb
blocks = []
RSpec.describe 'whatevz' do
  let(:a) { 123 }
  before { blocks << lambda { a } }
  2.times do
    example { blocks.each(&:call) if blocks.size == 2 }
  end
end
SPEC


# The spec passes
$ ruby --disable-gems -S bundle/bin/rspec spec/zomg_spec.rb
# ...
2 examples, 0 failures


# Back to the current code
$ git stash
Saved working directory and index state WIP on master: 35106b3 Merge pull request #1940 from rspec/tell-users-where-invalid-option-came-from
HEAD is now at 35106b3 Merge pull request #1940 from rspec/tell-users-where-invalid-option-came-from


#  The spec fails
$ ruby --disable-gems -S bundle/bin/rspec spec/zomg_spec.rb
# ...
2 examples, 1 failure

Also, I think this spec intends to verify it. But I don't understand the context well enough to really understand why it passes with this code deleted.

Might, further, be worth noting that when I also delete the @example_group_instance = nil code, that I still pass, except for a test that is explicitly checking for that.

$ git diff lib
diff --git a/lib/rspec/core/example.rb b/lib/rspec/core/example.rb
index c38a260..f61d2a8 100644
--- a/lib/rspec/core/example.rb
+++ b/lib/rspec/core/example.rb
@@ -224,11 +224,6 @@ module RSpec
           end
         rescue Exception => e
           set_exception(e)
-        ensure
-          ExampleGroup.each_instance_variable_for_example(@example_group_instance) do |ivar|
-            @example_group_instance.instance_variable_set(ivar, nil)
-          end
-          @example_group_instance = nil
         end

         finish(reporter)


$ ruby --disable-gem -S bundle/bin/rspec
  1) RSpec::Core::Example#run sets its reference to the example group instance to nil
     Failure/Error: expect(group.examples.first.instance_variable_get("@example_group_instance")).to be_nil
       expected: nil
            got: #<RSpec::ExampleGroups::Anonymous_120 "example" (./spec/rspec/core/example_spec.rb:256)>
     # ./spec/rspec/core/example_spec.rb:259:in `block (3 levels) in <top (required)>'
# ...
1698 examples, 1 failure, 1 pending

[dchelimsky]

Sorry for not responding sooner - didn't realize I had been mentioned here - but no, I don't remember :)

[myronmarston]

Looks like it does both, but my understanding is questionable.

Yep, it does both. My question was why both were done and not just the example group instance.

Also, I think this spec intends to verify it. But I don't understand the context well enough to really understand why it passes with this code deleted.

I believe the spec passes because it instantiates a new group instance and looks for the ivars on that. That spec, as originally written in b51be2c, did not do that. Looks like the extra group.new happened in 9189a22.

Might, further, be worth noting that when I also delete the @example_group_instance = nil code, that I still pass, except for a test that is explicitly checking for that.

Yep.

So here's my thoughts on what we should do:

• Replace the two specs @JoshCheek referenced with a new one that tests for the behavior of memory leaks rather than the mechanics of how memory leaks are avoided (as the two current specs do). I think such a spec could be written using ObjectSpace to manually track objects after a forced GC. I think that to properly demonstrate the memory leak addressed in b51be2c, the spec would have to use a real reporter rather than a NullReporter. The reporter holds references to all executed RSpec::Core::Example instances, so if those instances retain references to objects created while being run, we'll leak memory.
• Add a spec for the NoMethodError/let bug.
• Update the implementation to clear the example group instance but not the other ivars (that should hopefully pass both new specs).

@JoshCheek, do you want to take a stab at this since you've already started looking into it? Should be a much quicker change than your earlier PR :).

[JoshCheek]

@JoshCheek, do you want to take a stab at this since you've already started looking into it? Should be a much quicker change than your earlier PR :).

lol, sure (my bad on missing this, was a bit crazy last week :P)

[JoshCheek]

Uhm... lol, I kept squashing and force pushing, which I don't normally do. Hope y'all's inboxes don't get spammed >.<