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.

Sign up for GitHub

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
merged 2 commits into from
Nov 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8626be
Implement `flock_*` fiber-aware, without blocking the thread
straight-shoota Nov 5, 2022
5b34a25
Reword tests
straight-shoota Nov 18, 2022
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
38 changes: 36 additions & 2 deletions spec/std/file_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,10 +940,10 @@ describe "File" do
File.open(datapath("test_file.txt")) do |file1|
File.open(datapath("test_file.txt")) do |file2|
file1.flock_exclusive do
# BUG: check for EWOULDBLOCK when exception filters are implemented
expect_raises(IO::Error, "Error applying or removing file lock") do
exc = expect_raises(IO::Error, "Error applying file lock: file is already locked") do
file2.flock_exclusive(blocking: false) { }
end
exc.os_error.should eq Errno::EWOULDBLOCK
end
end
end
Expand All @@ -958,6 +958,40 @@ describe "File" do
end
end
end

pending_win32 "#flock_shared soft blocking fiber" do
File.open(datapath("test_file.txt")) do |file1|
File.open(datapath("test_file.txt")) do |file2|
done = Channel(Nil).new
file1.flock_exclusive

spawn do
file1.flock_unlock
done.send nil
end

file2.flock_shared
done.receive
end
end
end

pending_win32 "#flock_exclusive soft blocking fiber" do
File.open(datapath("test_file.txt")) do |file1|
File.open(datapath("test_file.txt")) do |file2|
done = Channel(Nil).new
file1.flock_exclusive

spawn do
file1.flock_unlock
done.send nil
end

file2.flock_exclusive
done.receive
end
end
end
end

it "reads at offset" do
Expand Down
25 changes: 20 additions & 5 deletions src/crystal/system/unix/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,14 +252,29 @@ module Crystal::System::File
flock LibC::FlockOp::UN
end

private def flock(op : LibC::FlockOp, blocking : Bool = true)
op |= LibC::FlockOp::NB unless blocking
private def flock(op : LibC::FlockOp, retry : Bool) : Nil
op |= LibC::FlockOp::NB

if LibC.flock(fd, op) != 0
raise IO::Error.from_errno("Error applying or removing file lock")
if retry
until flock(op)
::Fiber.yield
Copy link

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.

Copy link

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.

end
else
flock(op) || raise IO::Error.from_errno("Error applying file lock: file is already locked")
end
end

nil
private def flock(op) : Bool
if 0 == LibC.flock(fd, op)
true
else
errno = Errno.value
if errno.in?(Errno::EAGAIN, Errno::EWOULDBLOCK)
false
else
raise IO::Error.from_os_error("Error applying or removing file lock", errno)
end
end
end

private def system_fsync(flush_metadata = true) : Nil
Expand Down
1 change: 0 additions & 1 deletion src/io/file_descriptor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ class IO::FileDescriptor < IO
end

# TODO: use fcntl/lockf instead of flock (which doesn't lock over NFS)
# TODO: always use non-blocking locks, yield fiber until resource becomes available

def flock_shared(blocking = true)
flock_shared blocking
Expand Down