Ensure all request hooks are only called once per request.

Previously, the before_http_request hooks were being called twice for one request in some cases when using FakeWeb and Net::HTTP.  Also, in the VCR test environment there could be many hook invocations from WebMock/Typhoeus due to double-registration of hooks and such.

lib/vcr/library_hooks/fakeweb.rb

@@ -3,6 +3,7 @@
 require 'net/http'
 require 'vcr/extensions/net_http_response'
 require 'vcr/request_handler'
+require 'set'
 
 VCR::VersionChecker.new('FakeWeb', FakeWeb::VERSION, '1.3.0', '1.3').check_version!
 
@@ -23,8 +24,29 @@ def handle
           invoke_after_request_hook(@vcr_response) unless @recursing
         end
 
+        class << self
+          def already_seen_requests
+            @already_seen_requests ||= Set.new
+          end
+        end
+
       private
 
+        def invoke_before_request_hook
+          unless self.class.already_seen_requests.include?(request.object_id)
+            super
+            # we use the object_id so that if there is bug that causes
+            # us not to fully cleanup, we'll only be leaking the memory
+            # of one integer, not the whole request object.
+            self.class.already_seen_requests << request.object_id
+          end
+        end
+
+        def invoke_after_request_hook(vcr_response)
+          self.class.already_seen_requests.delete(request.object_id)
+          super
+        end
+
         def on_recordable_request
           perform_request(net_http.started?, :record_interaction)
         end

spec/support/shared_example_groups/hook_into_http_library.rb

@@ -2,7 +2,7 @@
 
 NET_CONNECT_NOT_ALLOWED_ERROR = /An HTTP request has been made that VCR does not know how to handle/
 
-shared_examples_for "a hook into an HTTP library" do |library, *other|
+shared_examples_for "a hook into an HTTP library" do |library_hook_name, library, *other|
   include HeaderDowncaser
 
   def interactions_from(file)
@@ -117,90 +117,61 @@ def self.test_playback(description, url)
       test_playback "with an encoded ampersand",        "http://example.com:80/search?q=#{CGI.escape("Q&A")}"
     end
 
-    context 'when there is a before_http_request hook' do
-      let(:string_in_cassette) { 'example.com get response 1 with path=foo' }
-
-      it 'plays back the cassette when a request is made' do
-        VCR.configure do |c|
-          c.cassette_library_dir = File.join(VCR::SPEC_ROOT, 'fixtures')
-          c.before_http_request do |request|
-            VCR.insert_cassette('fake_example_responses', :record => :none)
-          end
-        end
-        get_body_string(make_http_request(:get, 'http://example.com/foo')).should eq(string_in_cassette)
-      end
-
-      it 'yields the request to the hook' do
-        request = nil
-        VCR.configure do |c|
-          c.ignore_request { |r| true }
-          c.before_http_request { |r| request = r }
-        end
-        url = "http://localhost:#{VCR::SinatraApp.port}/foo"
-        make_http_request(:get, url)
-        request.method.should be(:get)
-        request.uri.should eq(url)
-      end
-
-      it 'does not get invoked if the library hook is disabled' do
-        VCR.library_hooks.should respond_to(:disabled?)
-        VCR.library_hooks.stub(:disabled? => true)
-
-        hook_called = false
-        VCR.configure do |c|
-          c.ignore_request { |r| true }
-          c.before_http_request { |r| hook_called = true }
-        end
-
-        make_http_request(:get, "http://localhost:#{VCR::SinatraApp.port}/foo")
-        hook_called.should be_false
-      end
-    end
-
-    context 'when there is an after_http_request hook' do
-      context 'when the request is ignored' do
+    describe "request hooks" do
+      context "when the request is ignored" do
         before(:each) do
           VCR.configuration.ignore_request { |r| true }
         end
 
-        it_behaves_like "after_http_request hook"
+        it_behaves_like "request hooks", library_hook_name
       end
 
       context 'when the request is recorded' do
         let!(:inserted_cassette) { VCR.insert_cassette('new_cassette') }
 
-        it_behaves_like "after_http_request hook" do
-          it 'can be used to eject a cassette after the request is recorded' do
-            VCR.configuration.after_http_request do |request|
-              VCR.eject_cassette
+        it_behaves_like "request hooks", library_hook_name do
+          let(:string_in_cassette) { 'example.com get response 1 with path=foo' }
+
+          it 'plays back the cassette when a request is made' do
+            VCR.eject_cassette
+            VCR.configure do |c|
+              c.cassette_library_dir = File.join(VCR::SPEC_ROOT, 'fixtures')
+              c.before_http_request do |request|
+                VCR.insert_cassette('fake_example_responses', :record => :none)
+              end
             end
+            get_body_string(make_http_request(:get, 'http://example.com/foo')).should eq(string_in_cassette)
+          end
+
+          specify 'the after_http_request hook can be used to eject a cassette after the request is recorded' do
+            VCR.configuration.after_http_request { |request| VCR.eject_cassette }
 
             VCR.should_receive(:record_http_interaction) do |interaction|
               VCR.current_cassette.should be(inserted_cassette)
             end
 
-            make_http_request(:get, request_url)
+            make_request
             VCR.current_cassette.should be_nil
           end
         end
       end
 
-      context 'when the request is played back' do
-        it_behaves_like "after_http_request hook" do
-          let(:request)       { VCR::Request.new(:get, request_url) }
-          let(:response_body) { "FOO!" }
-          let(:response)      { VCR::Response.new(status, nil, response_body, '1.1') }
-          let(:status)        { VCR::ResponseStatus.new(200, 'OK') }
-          let(:interaction)   { VCR::HTTPInteraction.new(request, response) }
+      context 'when a stubbed response is played back for the request' do
+        let(:request)       { VCR::Request.new(:get, request_url) }
+        let(:response_body) { "FOO!" }
+        let(:response)      { VCR::Response.new(status, nil, response_body, '1.1') }
+        let(:status)        { VCR::ResponseStatus.new(200, 'OK') }
+        let(:interaction)   { VCR::HTTPInteraction.new(request, response) }
 
-          before(:each) do
-            stub_requests([interaction], [:method, :uri])
-          end
+        before(:each) do
+          stub_requests([interaction], [:method, :uri])
         end
+
+        it_behaves_like "request hooks", library_hook_name
       end
 
       context 'when the request is not allowed' do
-        it_behaves_like "after_http_request hook" do
+        it_behaves_like "request hooks", library_hook_name do
           undef assert_expected_response
           def assert_expected_response(response)
             response.should be_nil

spec/support/shared_example_groups/request_hooks.rb

@@ -0,0 +1,58 @@
+shared_examples_for "request hooks" do |library_hook_name|
+  let(:request_url) { "http://localhost:#{VCR::SinatraApp.port}/foo" }
+
+  before(:each) do
+    # ensure that all the other library hooks are disabled so that we don't
+    # get double-hookage (such as for WebMock and Typhoeus both invoking the
+    # hooks for a typhoeus request)
+    VCR.library_hooks.stub(:disabled?) { |lib_name| lib_name != library_hook_name }
+  end
+
+  def make_request(disabled = false)
+    make_http_request(:get, request_url)
+  end
+
+  def assert_expected_response(response)
+    response.status.code.should eq(200)
+    response.body.should eq('FOO!')
+  end
+
+  [:before_http_request, :after_http_request].each do |hook|
+    specify "the #{hook} hook is only called once per request" do
+      call_count = 0
+      VCR.configuration.send(hook) { |r| call_count += 1 }
+
+      make_request
+      call_count.should eq(1)
+    end
+
+    specify "the #{hook} hook yields the request" do
+      request = nil
+      VCR.configuration.send(hook) { |r| request = r }
+
+      make_request
+      request.method.should be(:get)
+      request.uri.should eq(request_url)
+    end
+
+    specify "the #{hook} hook is not called if the library hook is disabled" do
+      VCR.library_hooks.should respond_to(:disabled?)
+      VCR.library_hooks.stub(:disabled? => true)
+
+      hook_called = false
+      VCR.configuration.send(hook) { |r| hook_called = true }
+
+      make_request(:disabled)
+      hook_called.should be_false
+    end
+  end
+
+  specify "the after_http_request hook yields the response if there is one and the second block arg is given" do
+    response = nil
+    VCR.configuration.after_http_request { |req, res| response = res }
+
+    make_request
+    assert_expected_response(response)
+  end
+end
+

spec/vcr/library_hooks/excon_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe "Excon hook" do
-  it_behaves_like 'a hook into an HTTP library', 'excon', :status_message_not_exposed
+  it_behaves_like 'a hook into an HTTP library', :excon, 'excon', :status_message_not_exposed
 
   it_performs('version checking', 'Excon',
     :valid    => %w[ 0.6.5 0.6.99 ],
@@ -68,7 +68,7 @@
       }.to raise_error(Excon::Errors::Error)
     end
 
-    it_behaves_like "after_http_request hook" do
+    it_behaves_like "request hooks", :excon do
       undef make_request
       def make_request(disabled = false)
         expect {

spec/vcr/library_hooks/fakeweb_spec.rb

@@ -1,7 +1,16 @@
 require 'spec_helper'
 
 describe "FakeWeb hook", :with_monkey_patches => :fakeweb do
-  it_behaves_like 'a hook into an HTTP library', 'net/http'
+  it_behaves_like 'a hook into an HTTP library', :fakeweb, 'net/http' do
+    before(:each) do
+      VCR::LibraryHooks::FakeWeb::RequestHandler.already_seen_requests.clear
+    end
+
+    after(:each) do
+      # assert that we are cleaning up the global state after every request
+      VCR::LibraryHooks::FakeWeb::RequestHandler.already_seen_requests.to_a.should eq([])
+    end
+  end
 
   it_performs('version checking', 'FakeWeb',
     :valid    => %w[ 1.3.0 1.3.1 1.3.99 ],
@@ -107,7 +116,7 @@ def make_post_request
       VCR.configuration.ignore_request { |r| true }
     end
 
-    it_behaves_like "after_http_request hook" do
+    it_behaves_like "request hooks", :fakeweb do
       undef assert_expected_response
       def assert_expected_response(response)
         response.should be_nil

spec/vcr/library_hooks/typhoeus_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe "Typhoeus hook", :with_monkey_patches => :typhoeus do
-  it_behaves_like 'a hook into an HTTP library', 'typhoeus'
+  it_behaves_like 'a hook into an HTTP library', :typhoeus, 'typhoeus'
 
   def stub_callback_registration
     # stub the callback registration methods so we don't get a second

spec/vcr/library_hooks/webmock_spec.rb

@@ -2,7 +2,7 @@
 
 describe "WebMock hook", :with_monkey_patches => :webmock do
   %w[net/http patron httpclient em-http-request curb typhoeus].each do |lib|
-    it_behaves_like 'a hook into an HTTP library', lib do
+    it_behaves_like 'a hook into an HTTP library', :webmock, lib do
       if lib == 'net/http'
         def normalize_request_headers(headers)
           headers.merge(DEFAULT_REQUEST_HEADERS)
@@ -19,6 +19,7 @@ def normalize_request_headers(headers)
 
     def stub_callback_registration
       ::WebMock.stub(:after_request)
+      ::WebMock.stub(:globally_stub_request)
     end
 
     def stub_version(version)

spec/vcr/middleware/faraday_spec.rb

@@ -3,7 +3,7 @@
 
 describe VCR::Middleware::Faraday do
   %w[ typhoeus net_http patron ].each do |lib|
-    it_behaves_like 'a hook into an HTTP library', "faraday (w/ #{lib})",
+    it_behaves_like 'a hook into an HTTP library', :faraday, "faraday (w/ #{lib})",
       :status_message_not_exposed,
       :does_not_support_rotating_responses,
       :not_disableable
@@ -29,7 +29,7 @@
     let(:connection)         { ::Faraday.new { |b| b.adapter :typhoeus } }
     let!(:inserted_cassette) { VCR.insert_cassette('new_cassette') }
 
-    it_behaves_like "after_http_request hook" do
+    it_behaves_like "request hooks", :faraday do
       undef make_request
       def make_request(disabled = false)
         response = nil
@@ -40,9 +40,7 @@ def make_request(disabled = false)
       end
 
       it 'can be used to eject a cassette after the request is recorded' do
-        VCR.configuration.after_http_request do |request|
-          VCR.eject_cassette
-        end
+        VCR.configuration.after_http_request { |request| VCR.eject_cassette }
 
         VCR.should_receive(:record_http_interaction) do |interaction|
           VCR.current_cassette.should be(inserted_cassette)