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#fsync on Windows #9257

Merged
merged 1 commit into from Jun 3, 2020

Conversation

kubo
Copy link
Contributor

@kubo kubo commented May 9, 2020

This PR uses _commit to implement File#fsync on Windows.

Metadata is also flushed as fsync.

  • _commit calls FlushFileBuffers internally. (I checked it by looking the source code of crt installed with MSVC.)
  • In CreateFileW document:

    To ensure that the metadata is flushed to disk, use the FlushFileBuffers function.

@kubo kubo closed this May 31, 2020
@kubo kubo deleted the file-fsync-on-windows branch May 31, 2020 23:54
@kubo kubo restored the file-fsync-on-windows branch June 1, 2020 00:03
@kubo
Copy link
Contributor Author

kubo commented Jun 1, 2020

This issue was closed by mistake. I removed kubo:file-fsync-on-windows branch on my repository and it made this PR closed.

@jhass jhass reopened this Jun 1, 2020
@straight-shoota straight-shoota added this to the 0.35.0 milestone Jun 1, 2020
@bcardiff
Copy link
Member

bcardiff commented Jun 1, 2020

Why is it better to use _commit instead of the public API FlushFileBuffers ?

@kubo
Copy link
Contributor Author

kubo commented Jun 3, 2020

Why is it better to use _commit instead of the public API FlushFileBuffers ?

Does this sentence imply that _commit is not a public API?

@bcardiff
Copy link
Member

bcardiff commented Jun 3, 2020

I will rephrase. Why is _commit preferred instead of FlushFileBuffers? Since CreateFileW is used to create the handle and FlushFileBuffers is the suggested function to use, it seems strange to me to use _commit.

Does it do something else that is needed or they are aliases?

@kubo
Copy link
Contributor Author

kubo commented Jun 3, 2020

I use _commit because I think that crystal uses C-runtime low-level I/O mainly and uses Win32 API only when low-level I/O lacks ability. For example, File.open and File.delete use low-level I/O (_wopen and _wunlink), not Win32 API (CreateFileW and DeleteFileW). File.rename doesn't use low-level I/O because _rename lacks the ability to replace existing files. Well, this is just a guess. I may think too much.

I have no other reasons to use _commit. I feel no difference with the following code except error messages (based on errno or based on win32 error code) when the method fails.

  private def system_fsync(flush_metadata = true) : Nil
    if LibC.FlushFileBuffers(windows_handle) == 0
      raise IO::Error.from_winerror("Error syncing file")
    end
  end

Does it do something else that is needed or they are aliases?

_commit(fd) is same with FlushFileBuffers((HANDLE)_get_osfhandle(fd)) when error checking is omitted. As for the above crystal code, _get_osfhandle(fd) is called inside of windows_handle.

@bcardiff
Copy link
Member

bcardiff commented Jun 3, 2020

Ok, thanks for the explanation 🙇

@bcardiff bcardiff merged commit 946b4e6 into crystal-lang:master Jun 3, 2020
@RX14
Copy link
Contributor

RX14 commented Jun 5, 2020

It's intended that we migrate from the "half-posix" file API to the full windows API. I used the unix-like wrappers only because of the ease of storing a fd instead of a HANDLE*, but this all needs refactoring to ideally use handles everywhere.

@kubo kubo deleted the file-fsync-on-windows branch June 6, 2020 14:27
@kubo kubo mentioned this pull request Jul 20, 2020
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants