-
-
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
Fix File#truncate
and #lock
for Win32 append-mode files
#14706
Fix File#truncate
and #lock
for Win32 append-mode files
#14706
Conversation
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.
issue: Non-blocking write needs to be handled as well.
The overwriting spec still breaks with File.open(filename, "a", blocking: false)
.
src/crystal/system/win32/file.cr
Outdated
private def write_blocking(handle, slice) | ||
if @system_append | ||
write_blocking(handle, slice) do | ||
overlapped = LibC::OVERLAPPED.new | ||
overlapped.union.offset.offset = 0xFFFFFFFF_u32 | ||
overlapped.union.offset.offsetHigh = 0xFFFFFFFF_u32 | ||
ret = LibC.WriteFile(handle, slice, slice.size, out bytes_written, pointerof(overlapped)) | ||
{ret, bytes_written} | ||
end | ||
else | ||
super | ||
end | ||
end |
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.
thought: Maybe we could reduce this duplication? The lib call is always the same, except for the overlapped struct.
I suppose it should be possible to have only a single implementation of write_blocking
with an additional parameter for append mode (could be a boolean flag, or the offset, or the entire overlapped struct).
module Crystal::System::FileDescriptor
private def write_blocking(handle, slice, append = false)
if append
overlapped = LibC::OVERLAPPED.new
overlapped.union.offset.offset = 0xFFFFFFFF_u32
overlapped.union.offset.offsetHigh = 0xFFFFFFFF_u32
end
write_blocking(handle, slice) do
ret = LibC.WriteFile(handle, slice, slice.size, out bytes_written, pointerof(overlapped))
{ret, bytes_written}
end
end
end
module Crystal::System::File
private def write_blocking(handle, slice)
write_blocking(handle, slice, @system_append)
end
end
This should make the diff smaller, the code has less duplication and is easier to comprehend.
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.
We could also consider moving @system_append
to Crystal::System::FileDescriptor
because that's where it matters. Might also make sense to expose it in FileDescriptor.new
to use this behaviour with an existing handle.
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.
That snippet wouldn't work as pointerof(overlapped)
should not be a Pointer(LibC::OVERLAPPED | Nil)
. Always passing an overlapped
doesn't work either as that makes all non-append writes occur at offset 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.
You're probably right that IO::FileDescriptor
could open a regular file via its #fd
directly and it would lose the append mode. I'm not sure if there is a way around that
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.
Sorry, I scribbled the code down too quickly. pointerof(overlapped)
should go in the if branch, of course.
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're probably right that
IO::FileDescriptor
could open a regular file via its#fd
directly and it would lose the append mode. I'm not sure if there is a way around that
There's probably no way around it. We could try to be smart and query the access mode of the file descriptor to figure out the intention. But we cannot change it anyway, so it's probably a mute point.
It's probably fine to accept that an external file descriptor without FILE_WRITE_DATA
access mode won't be able to truncate or lock.
But we can offer an option for a file descriptor with FILE_WRITE_DATA
to only append to the end of the file if we allow to configure @system_access
in the constructor.
Non-blocking regular files don't work without #14321, I would not worry about them here |
I figure the spec should pass with |
Co-authored-by: Johannes Müller <[email protected]>
Fixes #14702.