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

Implement flock_* fiber-aware, without blocking the thread #12728

Merged

Conversation

straight-shoota
Copy link
Member

Resolves #12721

spec/std/file_spec.cr Outdated Show resolved Hide resolved
spec/std/file_spec.cr Outdated Show resolved Hide resolved
@beta-ziliani
Copy link
Member

(changes are requested for the pending_win32 part, the name of the test is open to debate!)

@beta-ziliani beta-ziliani added this to the 1.7.0 milestone Nov 18, 2022
@straight-shoota straight-shoota merged commit 949a8a9 into crystal-lang:master Nov 19, 2022
@straight-shoota straight-shoota deleted the feature/flock-blocking branch November 19, 2022 19:47
raise IO::Error.from_errno("Error applying or removing file lock")
if retry
until flock(op)
::Fiber.yield

Choose a reason for hiding this comment

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

If no other fibers have work to be done, what prevents this from spinning and using up 100% of the CPU until the file lock is obtained?

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. A sleep 5.milliseconds will significantly lower the pressure on the CPU in such scenario without blocking for too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's definitely not ideal. This is just a basic first step and we're missing other features like a timeout (as suggested in #12721 (comment)). Timeout and sleep are related, so might make sense to consider that together.

Choose a reason for hiding this comment

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

Do we know when the timeout feature will be available in the crystal release for this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I'm not sure about the timeout. Maybe that should rather be handled more generically.

We should definitely sleep the fiber, though. I've posted a patch for that in #12861.

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

Successfully merging this pull request may close these issues.

Fiber-aware blocking flock
4 participants