rspec / rspec-support

Common code needed by the other RSpec gems. Not intended for direct use.
https://rspec.info
MIT License
99 stars 103 forks source link

Use `Fiber.current` for current execution context. Fixes #501. #502

Closed ioquatix closed 3 years ago

ioquatix commented 3 years ago

@eregon wdyt?

ioquatix commented 3 years ago

I'm not sure what the best option is to resolve the failure - do we add fiber to the list of exceptions?

pirj commented 3 years ago

the best option is to resolve the failure

My guess is that it compares loaded features to a clean process. It's a defensive measure to avoid loading anything explicitly, including 'fiber'.

ioquatix commented 3 years ago

I would suggest that if monitor worked, you should use it, but it's broken by the same bug. Ideally we fix this in 3.0.2

eregon commented 3 years ago

The change looks good to me.

Unfortunately there is no way to get Fiber.current without require 'fiber' in Ruby code (maybe we should change that in Ruby). fiber is not a default gem or anything you could replace, so requiring fiber seems fairly safe to me. It does have the downside that a gem might appear to work when running specs, but not when used outside of specs, e.g., if it uses Fiber.current, Fiber#alive? or Fiber#transfer (fairly unlikely I'd say). That's a general issue though, any test dependency might require stdlibs, and IMHO there should be at least one integration test using a subprocess to make sure this isn't an issue while loading the gem's files.

Using monitor seems best when available, but that's also a stdlib. monitor is also not a default gem, since it is a dependency of RubyGems. Because RubyGems already requires it, I think there is little problems to require it & use it in RSpec.

pirj commented 3 years ago

@eregon We don't require Rubygems as well, and even explicitly avoid loading them on CI. Rails, e.g. refers to Rubygems in their to securely compare gem versions, and don't explicitly require it that would break if Rails is run with Rubygems disabled with rubyopt. We deliberately don't require anything from stdlib unless absolutely necessary to prevent such breakages.

eregon commented 3 years ago

I think one way to avoid needing any stdlib here is to use Mutex#owned? and remove the @owner field entirely. Then neither Thread.current or Fiber.current would be used, and it'd be correct.

pirj commented 3 years ago

@eregon Amazing, thank you! @ioquatix Would #owned? fix your failing case?

ioquatix commented 3 years ago

I can try it out this week and report back

eregon commented 3 years ago

I made a PR using Mutex#owned?: https://github.com/rspec/rspec-support/pull/503

ioquatix commented 3 years ago

I defer to @eregon thanks for your help here.