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

[ioquatix]

No description provided.

[ioquatix]

@eregon wdyt?

[ioquatix]

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

[pirj]

We're dropping old rubies (< 2.3) in RSpec 4.
What are your primary targets? Do you still plan to support old rubies with pre-4.0 RSpec?

Does it make sense for RSpec to remove ReentrantMutex and use Monitor?
Or is it the case that Monitor has that same bug with Fiber.yield inside synchronize, and we'd better keep ReentrantMutex with what we'll end up doing in this PR that would work with any Ruby version? And the plan is to keep it until we drop support for Ruby 3.0?

[pirj]

I foresee that @JonRowe would say that requiring a dependency from stdlib is something that we try to avoid at all costs.

[ioquatix]

Fair enough.

[JonRowe]

Yes, we don't require things that developers need to require because it causes weird failures for developers. (e.g. RSpec will pass because we require it but it won't work in dev/production).

[pirj]

WDYT of

def current
  defined?(Fiber.current) ? Fiber.current : Thread.current
end

and using it?

[ioquatix]

It's broken.

[pirj]

Can you please elaborate?

[eregon]

Whenever some file would require 'fiber' the behavior and what's stored here would switch and it'd be a total mess.
Also Thread.current is not good enough if there is any non-root Fiber (any Fiber.new).

[pirj]

some file would require 'fiber' the behavior and what's stored here would switch and it'd be a total mess

For sure. I hope they don't do it dynamically when there are existing fibers and reentrant mutexes in action. Otherwise - do you see any issue?

Ok, let's put it another way. I couldn't find any mention of Fiber in stdlib docs. Core docs claim that in order to user current/transfer/alive? one has to require 'fiber'.
Unlike it is with require 'rubygems' or require 'date', require 'set' etc no constants are loaded, and it's not a problem if we load it from RSpec.

[JonRowe]

Would this not break in a more typical threaded solution? E.g. JRuby

[eregon]

I hope they don't do it dynamically when there are existing fibers and reentrant mutexes in action.

I don't think it's advisable to take that risk/assumption, and anyway AFAIK there is no need.

[pirj]

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]

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]

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]

@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]

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]

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

[ioquatix]

I can try it out this week and report back

[eregon]

I made a PR using Mutex#owned?: #503

[ioquatix]

I defer to @eregon thanks for your help here.