rspec / rspec-support

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

This is not thread local, it is in fact fiber local. #580

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago

https://github.com/rspec/rspec-support/blob/6d33eaa48a761d616e23b81fc33b68a6e68ec296/lib/rspec/support.rb#L93

I suggest introducing an actual thread local variable, e.g.

class Thread
  attr_accessor :rspec_state
end
ioquatix commented 1 year ago

See https://github.com/socketry/async-rspec/pull/22 for an example of where this can be a problem.

pirj commented 1 year ago

If I understand the docs correctly, it's suggested to use:

Thread.current.thread_variable_set(:foo, 1)
Thread.current.thread_variable_get(:foo)

to share the value between all fibers of the same thread. So we would have to:

- Thread.current[:__rspec] ||= {}
+ Thread.current.thread_variable_get(:__rspec) || Thread.current.thread_variable_set(:__rspec, {})

Does this sound right, @ioquatix ?

ioquatix commented 1 year ago

I think you should do the same as I proposed here: https://github.com/ruby/debug/pull/987

Such an approach was also adopted recently by Rails.

I've been an advocate of this style for a while and I think it's good to be explicit. If you want the details I am happy to write them down; LMK.

So:

class Thread
  attr_accessor :rspec_state
end

module RSpec::Support
  def self.current_state
    Thread.current.rspec_state ||= {}
  end
end

That's assuming you will always only run one test per thread at a time.

This is also the most efficient way to do TLS, i.e. it works really well with the recent work on object shapes.

JonRowe commented 1 year ago

We try to avoid monkey patching as much as possible, so a solution which doesn't modify the thread class is preferred.

ioquatix commented 1 year ago

I wouldn't really call this a monkey patch. This is the most efficient and correct way to add a thread-local variable (we don't have any mechanism for annotating a global variable as thread local unfortunately), it also prevents clobbering as clobbering an attr_accessor will issue a warning which isn't the case for Thread.current.thread_variable_set.

JonRowe commented 1 year ago

If Thread.current.thread_variable_get(:__rspec) || Thread.current.thread_variable_set(:__rspec, {}) works we'll make that patch.

pirj commented 1 year ago

assuming you will always only run one test per thread at a time

I wouldn’t rely on this assumption in the long run

ioquatix commented 1 year ago

The alternative is to use Fiber.storage which is inherited across descendent threads and fibers.

pirj commented 1 year ago

We have to figure out something that would also work on 1.8 and 1.9 And we can add a note to replace with a modern API in RSpec 4

JonRowe commented 1 year ago

Fix released in 3.12.1 for any Ruby >= 2, older Rubies will maintain existing behaviour.