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 File.tempfile in Crystal #12111

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
71 changes: 71 additions & 0 deletions spec/std/file/tempfile_spec.cr
Original file line number Diff line number Diff line change
@@ -1,4 +1,34 @@
require "../spec_helper"
require "../../support/tempfile"

private class TestRNG(T)
include Random

def initialize(@data : Array(T))
@i = 0
end

def next_u : T
i = @i
@i = (i + 1) % @data.size
@data[i]
end

def reset
@i = 0
end
end

private def normalize_permissions(permissions, *, directory)
{% if flag?(:win32) %}
normalized_permissions = 0o444
normalized_permissions |= 0o222 if permissions.bits_set?(0o200)
normalized_permissions |= 0o111 if directory
File::Permissions.new(normalized_permissions)
{% else %}
File::Permissions.new(permissions)
{% end %}
end

describe File do
describe ".tempname" do
Expand Down Expand Up @@ -42,6 +72,7 @@ describe File do
it "creates and writes" do
tempfile = File.tempfile
tempfile.print "Hello!"
tempfile.info.permissions.should eq normalize_permissions(0o600, directory: false)
tempfile.close

File.exists?(tempfile.path).should be_true
Expand All @@ -53,6 +84,7 @@ describe File do
it "accepts single suffix argument" do
tempfile = File.tempfile ".bar"
tempfile.print "Hello!"
tempfile.info.permissions.should eq normalize_permissions(0o600, directory: false)
tempfile.close

File.extname(tempfile.path).should eq(".bar")
Expand All @@ -66,6 +98,7 @@ describe File do
it "accepts prefix and suffix arguments" do
tempfile = File.tempfile "foo", ".bar"
tempfile.print "Hello!"
tempfile.info.permissions.should eq normalize_permissions(0o600, directory: false)
tempfile.close

File.extname(tempfile.path).should eq(".bar")
Expand Down Expand Up @@ -156,3 +189,41 @@ describe File do
end
end
end

describe Crystal::System::File do
describe ".mktemp" do
it "creates random file name" do
with_tempfile "random-path" do |tempdir|
Dir.mkdir tempdir
fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
path.should eq Path[tempdir, "A789abcdeZ"].to_s
ensure
File.from_fd(path, fd).close if fd && path
end
end

it "retries when file exists" do
with_tempfile "retry" do |tempdir|
Dir.mkdir tempdir
existing_path = Path[tempdir, "A789abcdeZ"]
File.touch existing_path
fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22]))
path.should eq File.join(tempdir, "AfghijklmZ")
ensure
File.from_fd(path, fd).close if fd && path
end
end

it "raises when no valid path is found" do
with_tempfile "random-path" do |tempdir|
Dir.mkdir tempdir
File.touch Path[tempdir, "A789abcdeZ"]
expect_raises(File::AlreadyExistsError, "Error creating temporary file") do
fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
ensure
File.from_fd(path, fd).close if fd && path
end
end
end
end
end
37 changes: 37 additions & 0 deletions src/crystal/system/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,43 @@ module Crystal::System::File
m | o
end

LOWER_ALPHANUM = "0123456789abcdefghijklmnopqrstuvwxyz".to_slice
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be private?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the Crystal::System namespace, so inherently non-public.


def self.mktemp(prefix : String?, suffix : String?, dir : String, random : ::Random = ::Random::DEFAULT) : {LibC::Int, String}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the name generation logic here needs to be distinct from ::File.tempname? That method also includes the process ID and the current time. Could we include them here? Could we get rid of them there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I considered the implications for tempname that but it's probably better done in a follow up to keep things simple.

mode = LibC::O_RDWR | LibC::O_CREAT | LibC::O_EXCL
perm = ::File::Permissions.new(0o600)

prefix = ::File.join(dir, prefix || "")
bytesize = prefix.bytesize + 8 + (suffix.try(&.bytesize) || 0)

100.times do
path = String.build(bytesize) do |io|
io << prefix
8.times do
io.write_byte LOWER_ALPHANUM.sample(random)
end
io << suffix
end

fd, errno = open(path, mode, perm)

if errno.none?
return {fd, path}
elsif error_is_file_exists?(errno)
# retry
next
else
raise ::File::Error.from_os_error("Error creating temporary file", errno, file: path)
end
end

raise ::File::AlreadyExistsError.new("Error creating temporary file", file: "#{prefix}********#{suffix}")
end

private def self.error_is_file_exists?(errno)
Errno.value.in?(Errno::EEXIST, WinError::ERROR_ALREADY_EXISTS)
end

# Closes the internal file descriptor without notifying libevent.
# This is directly used after the fork of a process to close the
# parent's Crystal::Signal.@@pipe reference before re initializing
Expand Down
31 changes: 12 additions & 19 deletions src/crystal/system/unix/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,25 @@ require "file/error"

# :nodoc:
module Crystal::System::File
def self.open(filename, mode, perm)
oflag = open_flag(mode) | LibC::O_CLOEXEC
def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions)
perm = ::File::Permissions.new(perm) if perm.is_a? Int32

fd = LibC.open(filename.check_no_null_byte, oflag, perm)
if fd < 0
raise ::File::Error.from_errno("Error opening file with mode '#{mode}'", file: filename)
fd, errno = open(filename, open_flag(mode), perm)

unless errno.none?
raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename)
end

fd
end

def self.mktemp(prefix, suffix, dir) : {LibC::Int, String}
prefix.try &.check_no_null_byte
suffix.try &.check_no_null_byte
dir.check_no_null_byte

dir = dir + ::File::SEPARATOR
path = "#{dir}#{prefix}.XXXXXX#{suffix}"
def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno}
filename.check_no_null_byte
flags |= LibC::O_CLOEXEC

if suffix
fd = LibC.mkstemps(path, suffix.bytesize)
else
fd = LibC.mkstemp(path)
end
fd = LibC.open(filename, flags, perm)

raise ::File::Error.from_errno("Error creating temporary file", file: path) if fd == -1
{fd, path}
{fd, fd < 0 ? Errno.value : Errno::NONE}
end

def self.info?(path : String, follow_symlinks : Bool) : ::File::Info?
Expand Down
4 changes: 0 additions & 4 deletions src/crystal/system/wasi/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ module Crystal::System::File
raise NotImplementedError.new "Crystal::System::File#flock"
end

def self.mktemp(prefix, suffix, dir) : {LibC::Int, String}
raise NotImplementedError.new "Crystal::System::File.mktemp"
end

def self.delete(path : String, *, raise_on_missing : Bool) : Bool
raise NotImplementedError.new "Crystal::System::File.delete"
end
Expand Down
20 changes: 7 additions & 13 deletions src/crystal/system/win32/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ require "c/handleapi"
module Crystal::System::File
def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions) : LibC::Int
perm = ::File::Permissions.new(perm) if perm.is_a? Int32
oflag = open_flag(mode) | LibC::O_BINARY | LibC::O_NOINHERIT

# Only the owner writable bit is used, since windows only supports
# the read only attribute.
if perm.owner_write?
Expand All @@ -19,24 +17,20 @@ module Crystal::System::File
perm = LibC::S_IREAD
end

fd = LibC._wopen(System.to_wstr(filename), oflag, perm)
if fd == -1
raise ::File::Error.from_errno("Error opening file with mode '#{mode}'", file: filename)
fd, errno = open(filename, open_flag(mode), ::File::Permissions.new(perm))
unless errno.none?
raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename)
end

fd
end

def self.mktemp(prefix : String?, suffix : String?, dir : String) : {LibC::Int, String}
path = "#{dir}#{::File::SEPARATOR}#{prefix}.#{::Random::Secure.hex}#{suffix}"
def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno}
flags |= LibC::O_BINARY | LibC::O_NOINHERIT

mode = LibC::O_RDWR | LibC::O_CREAT | LibC::O_EXCL | LibC::O_BINARY | LibC::O_NOINHERIT
fd = LibC._wopen(System.to_wstr(path), mode, ::File::DEFAULT_CREATE_PERMISSIONS)
if fd == -1
raise ::File::Error.from_errno("Error creating temporary file", file: path)
end
fd = LibC._wopen(System.to_wstr(filename), flags, perm)

{fd, path}
{fd, fd == -1 ? Errno.value : Errno::NONE}
end

NOT_FOUND_ERRORS = {
Expand Down
5 changes: 5 additions & 0 deletions src/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ class File < IO::FileDescriptor
super(fd, blocking)
end

# :nodoc:
def self.from_fd(path : String, fd : Int, *, blocking = false, encoding = nil, invalid = nil)
new(path, fd, blocking: blocking, encoding: encoding, invalid: invalid)
end

HertzDevil marked this conversation as resolved.
Show resolved Hide resolved
# Opens the file named by *filename*.
#
# *mode* must be one of the following file open modes:
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/aarch64-darwin/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ lib LibC
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
O_TRUNC = 0x0400
O_EXCL = 0x0800
O_APPEND = 0x0008
O_NONBLOCK = 0x0004
O_SYNC = 0x0080
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/aarch64-linux-gnu/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o100000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/aarch64-linux-musl/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o100000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/arm-linux-gnueabihf/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o100000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/i386-linux-gnu/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o400000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/i386-linux-musl/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o400000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/wasm32-wasi/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0
O_CREAT = 1_u16 << 12
O_EXCL = 4_u16 << 12
Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking, did you go through the system headers of all supported systems to grab those O_EXCL values?

Copy link
Member Author

@straight-shoota straight-shoota Feb 14, 2023

Choose a reason for hiding this comment

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

I don't recall doing that explicitly. That's the problem with ridiculously long review timespans (poke @beta-ziliani 😏).
But I'm pretty sure I must have. The values are different, and I don't think I rolled a dice 😆
Perhaps I didn't go through all original headers though and grabbed the values from some other implementation in Go instead. Can't say.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the WASI definition I could find: https://github.com/WebAssembly/wasi-libc/blob/8daaba387ce70a6088afe3916e9e16ed1c2bc630/libc-bottom-half/headers/public/__header_fcntl.h

Then it looks like Linux uses 0200 whereas the BSDs (including Darwin) use 0x0800. So those values are probably correct

O_NOFOLLOW = 0x01000000
O_TRUNC = 8_u16 << 12
O_APPEND = 1_u16
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-darwin/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ lib LibC
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
O_TRUNC = 0x0400
O_EXCL = 0x0800
O_APPEND = 0x0008
O_NONBLOCK = 0x0004
O_SYNC = 0x0080
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-dragonfly/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ lib LibC
F_SETFL = 4
FD_CLOEXEC = 1
O_CLOEXEC = 0x20000
O_EXCL = 0x0800
O_TRUNC = 0x0400
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-freebsd/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ lib LibC
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
O_TRUNC = 0x0400
O_EXCL = 0x0800
O_APPEND = 0x0008
O_NONBLOCK = 0x0004
O_SYNC = 0x0080
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-linux-gnu/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o400000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-linux-musl/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ lib LibC
FD_CLOEXEC = 1
O_CLOEXEC = 0o2000000
O_CREAT = 0o100
O_EXCL = 0o200
O_NOFOLLOW = 0o400000
O_TRUNC = 0o1000
O_APPEND = 0o2000
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-netbsd/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ lib LibC
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
O_TRUNC = 0x0400
O_EXCL = 0x0800
O_APPEND = 0x0008
O_NONBLOCK = 0x0004
O_SYNC = 0x0080
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-openbsd/c/fcntl.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ lib LibC
O_CREAT = 0x0200
O_NOFOLLOW = 0x0100
O_TRUNC = 0x0400
O_EXCL = 0x0800
O_APPEND = 0x0008
O_NONBLOCK = 0x0004
O_SYNC = 0x0080
Expand Down