-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature: File Atomic Write #6133
Feature: File Atomic Write #6133
Conversation
src/file.cr
Outdated
|
||
begin | ||
open(atomic_path, "w", perm, encoding, invalid) do |fd| | ||
fd.flock_exclusive() do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant ()
.
src/file.cr
Outdated
end | ||
end | ||
ensure | ||
if (success_flag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant ()
src/file.cr
Outdated
end | ||
end | ||
|
||
return success_flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant return
src/file.cr
Outdated
|
||
# :nodoc: | ||
protected def self.atomic_install(atomic_path : String, dest_path : String) : Nil | ||
if (info = info?(dest_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize brackets were redundant on assignment. Interesting.
src/file.cr
Outdated
protected def self.new_atomic_path(path : String, length : Int::Unsigned = 16_u8, limit : Int::Unsigned = 8_u8) : String | ||
atomic_path = "#{path}.atomic_#{Random::Secure.urlsafe_base64(length)}" | ||
|
||
while (exists?(atomic_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant ()
src/file.cr
Outdated
limit -= 1 | ||
end | ||
|
||
return atomic_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant return
src/file.cr
Outdated
atomic_path = "#{path}.atomic_#{Random::Secure.urlsafe_base64(length)}" | ||
|
||
while (exists?(atomic_path)) | ||
raise "Failed to generate temporary path." if (limit <= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception message IMO should mention the actual reason of the failure - without following dot .
.
Redundant ()
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise "string"
won't do, needs to be an exception class
src/file.cr
Outdated
atomic_write(path, perm, encoding, invalid) do |fd| | ||
case content | ||
when Bytes then fd.write(content) | ||
when IO then IO.copy(content, fd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making different method overloads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied File.write()
which uses this method. I suspect its for verbosity reasons. @asterite, @RX14, @straight-shoota, any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is shorter so I don't really care.
src/file.cr
Outdated
end | ||
end | ||
ensure | ||
if success_flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open(...) do |fd|
# ...
atomic_install(atomic_path, path)
rescue ex
delete(atomic_path)
raise ex
end
should work?
src/file.cr
Outdated
end | ||
end | ||
|
||
success_flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't return a bool: it always returns true or raises an exception.
src/file.cr
Outdated
protected def self.new_atomic_path(path : String, length : Int::Unsigned = 16_u8, limit : Int::Unsigned = 8_u8) : String | ||
atomic_path = "#{path}.atomic_#{Random::Secure.urlsafe_base64(length)}" | ||
|
||
while exists?(atomic_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just hardcode length = 16
, then you're using 128bits of random and can assume there's no clashes. Then you can just remove this method.
chmod(atomic_path, info.permissions) | ||
chown(atomic_path, info.owner, info.group) | ||
end | ||
rename(atomic_path, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this chmod/chown/rename be inside the rescue
block? (not the open
block)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, chmod/chown/rename should be done only if the write was successful (no exception)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I worded that terribly.
I meant inside a begin/end
block which has a rescue
block, instead of inside the rescue
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, in case chmod/chown fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
This should be in a shard. |
@asterite Why do you think so? |
@Sija Because I expected that to be a syscall that writes the file atomically. This is one way to do it: create a temp file (no control over the name), rename it, change permissions. All of that doesn't happen atomically. There's no such thing in Ruby, Go, etc. They are provided as packages/gems. It can be provided as a shard too. |
I largely agree. This isn't an OS primitive, so I think it's a bit too opinionated and special-case to be in the stdlib. |
So, @chris-huxtable, don't take this as a dismissal. Please do actually make a shard, it would be very useful :) |
I have written a shard, |
Of course, it would be better to not reopen a standard library module there |
@oprypin - One of the reasons I think it should be in the stdlib (not to mention I think atomic writes should probably be the default). This is similar to atomic_write, referenced above. I also don't prescribe to the philosophy which promotes the prohibition of class extension. I come from Objective-C which treats this as a positive feature of a language. That being said, I realize it can be improperly maintained. This also isn't really a good place to have this argument though. |
Should we close then? |
This PR adds
File.atomic_write()
to which prevents file corruption during a write.