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

Fix for Ruby 3.1: Use fiber-local var for global_hook_disabled_requests #907

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

jhawthorn
Copy link
Contributor

Previously the finalizer being registered failed on Ruby trunk because finalizers are passed an argument and the lambda didn't accept one.

I think it's simpler here to just use a fiber-local (we could use a thread-local, but my gut feeling here was to use fiber-local) variable and have our array cleaned up automatically when the thread exits without need of a finalizer.

Previously the finalizer being registered failed on Ruby trunk because
finalizers are passed an argument and the lambda didn't accept one.

I think it's simpler here to just use a fiber-local variable and have
our array cleaned up automatically when the thread exits without need of
a finalizer.
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@olleolleolle
Copy link
Member

olleolleolle commented Nov 18, 2021

@jhawthorn Would you be able to make a CHANGELOG line which summarizes this change?

(If you don't get around to, I'll get to it, at some point.)

@olleolleolle olleolleolle merged commit 98cc00b into vcr:master Nov 19, 2021
olleolleolle added a commit that referenced this pull request Nov 19, 2021
@nehresma
Copy link

With the release of Ruby 3.1.0 a few days ago, would it be possible to cut and release a new VCR release that includes this fix?

@rainerborene
Copy link

+1

@olleolleolle
Copy link
Member

olleolleolle commented Jan 11, 2022

Followin SemVer, that's a new major, hold on to your hats. (Well, Faraday changes made me register this #918, so any headgear-holding needs to continue for a little longer.)

@benhutton
Copy link

@olleolleolle is there a way to just ship this as a patch release on top of 6.0.0, without all of the other commits? Or is this specifically what is messing up Faraday? Or is VCR's integration with Faraday going to be problematic regardless?

It's just a shame to have this gate an otherwise clean upgrade to Ruby 3.1!

@donv
Copy link

donv commented Feb 14, 2022

@olleolleolle is there a way to just ship this as a patch release on top of 6.0.0, without all of the other commits? Or is this specifically what is messing up Faraday? Or is VCR's integration with Faraday going to be problematic regardless?

You can put this in your test_helper.rb after configuring VCR:

# FIXME(uwe): Remove when fixed
# https://github.com/vcr/vcr/pull/907/files
module VCR
  class LibraryHooks
    # @private
    module WebMock
      extend self

      def with_global_hook_disabled(request)
        global_hook_disabled_requests << request

        begin
          yield
        ensure
          global_hook_disabled_requests.delete(request)
        end
      end

      def global_hook_disabled?(request)
        requests = Thread.current[:_vcr_webmock_disabled_requests]
        requests && requests.include?(request)
      end

      def global_hook_disabled_requests
        Thread.current[:_vcr_webmock_disabled_requests] ||= []
      end
    end
  end
end
# EMXIF

@iGEL
Copy link

iGEL commented Feb 14, 2022

@benhutton

I just created this patch (to be placed in config/initializers/patch_vcr.rb:

# frozen_string_literal: true

if Rails.env.test?
  unless Gem.loaded_specs["vcr"].version == Gem::Version.create("6.0.0")
    raise "This patch is very likely already in the next VCR release after 6.0.0"
  end

  require "vcr/library_hooks/webmock"

  module VCR
    class LibraryHooks
      module WebMock
        def global_hook_disabled?(request)
          requests = Thread.current[:_vcr_webmock_disabled_requests]
          requests && requests.include?(request)
        end

        def global_hook_disabled_requests
          Thread.current[:_vcr_webmock_disabled_requests] ||= []
        end
      end
    end
  end
end

@donv Wow, pretty much the same idea, just minutes apart 😉

@danielricecodes
Copy link

@olleolleolle is there a way to just ship this as a patch release on top of 6.0.0, without all of the other commits? Or is this specifically what is messing up Faraday? Or is VCR's integration with Faraday going to be problematic regardless?

It's just a shame to have this gate an otherwise clean upgrade to Ruby 3.1!

+1

Since this was merged a while ago, there really needs to be a patch release here. In the meantime:

gem 'vcr', github: 'vcr/vcr' #Edge version for Ruby 3.1 support

🤷

@Justintime50
Copy link

Looks to be released as a part of VCR 6.1

@coldnebo
Copy link

@benhutton

I just created this patch (to be placed in config/initializers/patch_vcr.rb:

Thanks! I put this in my project, but I'm getting this error now, any ideas?

FrozenError:
  can't modify frozen Array: ["/home/app/.gem/gems/actiontext-7.0.4.3/app/helpers", "/home/app/.gem/gems/actiontext-7.0.4.3/app/models"]
# /home/app/.gem/gems/railties-7.0.4.3/lib/rails/engine.rb:575:in `unshift'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants