Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f874eba
Use `Fiber.current` for current execution context. Fixes #501.
ioquatix Apr 25, 2021
7ae351e
Add missing `require 'fiber'`.
ioquatix Apr 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/rspec/support/reentrant_mutex.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'fiber'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Member

Choose a reason for hiding this comment

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

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).


module RSpec
module Support
# Allows a thread to lock out other threads from a critical section of code,
Expand Down Expand Up @@ -28,8 +30,8 @@ def synchronize
private

def enter
@mutex.lock if @owner != Thread.current
@owner = Thread.current
@mutex.lock if @owner != Fiber.current
@owner = Fiber.current
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

WDYT of

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

and using it?

Copy link
Author

Choose a reason for hiding this comment

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

It's broken.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please elaborate?

Copy link
Contributor

@eregon eregon Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@count += 1
end

Expand Down