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

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

Closed
ioquatix opened this issue Jun 5, 2023 · 10 comments · Fixed by #581 · May be fixed by #610
Closed

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

ioquatix opened this issue Jun 5, 2023 · 10 comments · Fixed by #581 · May be fixed by #610

Comments

@ioquatix
Copy link

ioquatix commented Jun 5, 2023

Thread.current[:__rspec] ||= {}

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

class Thread
  attr_accessor :rspec_state
end
@ioquatix
Copy link
Author

ioquatix commented Jun 5, 2023

See socketry/async-rspec#22 for an example of where this can be a problem.

@pirj
Copy link
Member

pirj commented Jun 5, 2023

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
Copy link
Author

ioquatix commented Jun 6, 2023

I think you should do the same as I proposed here: ruby/debug#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
Copy link
Member

JonRowe commented Jun 6, 2023

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

@ioquatix
Copy link
Author

ioquatix commented Jun 6, 2023

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
Copy link
Member

JonRowe commented Jun 6, 2023

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

@pirj
Copy link
Member

pirj commented Jun 6, 2023

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
Copy link
Author

ioquatix commented Jun 6, 2023

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

@pirj
Copy link
Member

pirj commented Jun 6, 2023

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
Copy link
Member

JonRowe commented Jun 26, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment