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

Add specs for Fiber.blocking? #886

Merged
merged 3 commits into from
Oct 28, 2021
Merged

Conversation

HeroProtagonist
Copy link
Contributor

relates to #823

Fiber.blocking? tells whether the current execution context is
blocking. [Feature #16786]

@HeroProtagonist
Copy link
Contributor Author

@eregon 👋

I am a little confused by the failures.

fiber = Fiber.new { Fiber.current.blocking? }
blocking = fiber.resume
blocking.should == true

From the description of the feature. I would expect this to pass. However, it fails

A fiber created by Fiber.new(blocking: true) (the default Fiber.new) becomes a "blocking Fiber" and has no changes from current Fiber implementation

fiber = Fiber { Fiber.current.blocking? }
blocking = fiber.resume
blocking.should == false

Above causes NoMethodError. Has the fiber method been implemented?

We also introduce a new method which simplifes the creation of these non-blocking fibers:

@ioquatix
Copy link
Member

Matz rejected Fiber method so I don't know where that spec came from. Can you tell me? Thanks!

@HeroProtagonist
Copy link
Contributor Author

@ioquatix

🤦I made it to try and cover the different cases. I was only going off the description and did not read through the whole thread. Thanks for letting me know.

Now just need to figure out why the first case is not returning true

@ioquatix
Copy link
Member

I believe we changed the default to be non-blocking by default. Originally we wanted to preserve the existing behaviour but it was deemed safe enough to allow non-blocking by default.

@HeroProtagonist
Copy link
Contributor Author

Ah I see now https://docs.ruby-lang.org/en/master/Fiber.html#method-c-new.
Thanks for letting me know. Pushed the changes up. Hopefully all ✅ now.

core/fiber/blocking_spec.rb Outdated Show resolved Hide resolved
core/fiber/blocking_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks great, could you address my comments?

BTW, yes the CRuby bug tracker is not a great place to find out what was actually implemented unfortunately.

@HeroProtagonist
Copy link
Contributor Author

I think I covered all the cases and hopefully got the wording right 🤞

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

This looks awesome, thank you!

@eregon eregon merged commit 0b8be3e into ruby:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants