Feature: File Atomic Write #6133
Conversation
src/file.cr
Outdated
|
|
||
| begin | ||
| open(atomic_path, "w", perm, encoding, invalid) do |fd| | ||
| fd.flock_exclusive() do |
src/file.cr
Outdated
| end | ||
| end | ||
| ensure | ||
| if (success_flag) |
src/file.cr
Outdated
| end | ||
| end | ||
|
|
||
| return success_flag |
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.
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)) |
src/file.cr
Outdated
| limit -= 1 | ||
| end | ||
|
|
||
| return atomic_path |
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.
Exception message IMO should mention the actual reason of the failure - without following dot ..
Redundant () too.
There was a problem hiding this comment.
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.
Consider making different method overloads
There was a problem hiding this comment.
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.
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.
open(...) do |fd|
# ...
atomic_install(atomic_path, path)
rescue ex
delete(atomic_path)
raise ex
endshould work?
src/file.cr
Outdated
| end | ||
| end | ||
|
|
||
| success_flag |
There was a problem hiding this comment.
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.
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.
Shouldn't this chmod/chown/rename be inside the rescue block? (not the open block)
There was a problem hiding this comment.
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.
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.
You mean, in case chmod/chown fails?
|
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.