fix: windows implement of op::Write is incorrect (#294)

* fix: fix windows position read/write bug

Signed-off-by: Lzzzt <[email protected]>

* ci: make clippy happy

Signed-off-by: lzzzt <[email protected]>

---------

Signed-off-by: Lzzzt <[email protected]>
Signed-off-by: lzzzt <[email protected]>

Author: Lzzzzzt

monoio/src/driver/op/open.rs

@@ -79,9 +79,15 @@ impl OpAble for Open {
 
     #[cfg(all(any(feature = "legacy", feature = "poll-io"), windows))]
     fn legacy_call(&mut self) -> io::Result<u32> {
+        use std::{ffi::OsString, os::windows::ffi::OsStrExt};
+
+        let os_str = OsString::from(self.path.to_string_lossy().into_owned());
+
+        // Convert OsString to wide character format (Vec<u16>).
+        let wide_path: Vec<u16> = os_str.encode_wide().chain(Some(0)).collect();
         syscall!(
             CreateFileW(
-                self.path.as_c_str().as_ptr().cast(),
+                wide_path.as_ptr(),
                 self.opts.access_mode()?,
                 self.opts.share_mode,
                 self.opts.security_attributes,

monoio/src/driver/op/read.rs

@@ -10,7 +10,7 @@ use {
     windows_sys::Win32::{
         Foundation::TRUE,
         Networking::WinSock::{WSAGetLastError, WSARecv, WSAESHUTDOWN},
-        Storage::FileSystem::{ReadFile, SetFilePointer, FILE_CURRENT, INVALID_SET_FILE_POINTER},
+        Storage::FileSystem::{ReadFile, SetFilePointer, FILE_BEGIN, INVALID_SET_FILE_POINTER},
     },
 };
 
@@ -111,7 +111,9 @@ impl<T: IoBufMut> OpAble for Read<T> {
         let ret = unsafe {
             // see https://learn.microsoft.com/zh-cn/windows/win32/api/fileapi/nf-fileapi-setfilepointer
             if seek_offset != 0 {
-                let r = SetFilePointer(fd, seek_offset, std::ptr::null_mut(), FILE_CURRENT);
+                // We use `FILE_BEGIN` because this behavior should be the same with unix syscall
+                // `pwrite`, which uses the offset from the begin of the file.
+                let r = SetFilePointer(fd, seek_offset, std::ptr::null_mut(), FILE_BEGIN);
                 if INVALID_SET_FILE_POINTER == r {
                     return Err(io::Error::last_os_error());
                 }

monoio/src/driver/op/write.rs

@@ -8,7 +8,7 @@ use io_uring::{opcode, types};
 use windows_sys::Win32::{
     Foundation::TRUE,
     Networking::WinSock::WSASend,
-    Storage::FileSystem::{SetFilePointer, WriteFile, FILE_CURRENT, INVALID_SET_FILE_POINTER},
+    Storage::FileSystem::{SetFilePointer, WriteFile, FILE_BEGIN, INVALID_SET_FILE_POINTER},
 };
 
 use super::{super::shared_fd::SharedFd, Op, OpAble};
@@ -95,7 +95,9 @@ impl<T: IoBuf> OpAble for Write<T> {
         let ret = unsafe {
             // see https://learn.microsoft.com/zh-cn/windows/win32/api/fileapi/nf-fileapi-setfilepointer
             if seek_offset != 0 {
-                let r = SetFilePointer(fd, seek_offset, std::ptr::null_mut(), FILE_CURRENT);
+                // We use `FILE_BEGIN` because this behavior should be the same with unix syscall
+                // `pwrite`, which uses the offset from the begin of the file.
+                let r = SetFilePointer(fd, seek_offset, std::ptr::null_mut(), FILE_BEGIN);
                 if INVALID_SET_FILE_POINTER == r {
                     return Err(io::Error::last_os_error());
                 }

monoio/src/fs/mod.rs

@@ -30,6 +30,9 @@ pub use file_type::FileType;
 
 #[cfg(unix)]
 mod permissions;
+#[cfg(windows)]
+use std::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle};
+
 #[cfg(unix)]
 pub use permissions::Permissions;
 
@@ -38,16 +41,20 @@ use crate::buf::IoBuf;
 use crate::driver::op::Op;
 
 /// Read the entire contents of a file into a bytes vector.
-#[cfg(unix)]
 pub async fn read<P: AsRef<Path>>(path: P) -> io::Result<Vec<u8>> {
-    use std::os::fd::{AsRawFd, FromRawFd, IntoRawFd};
-
     use crate::buf::IoBufMut;
 
     let file = File::open(path).await?;
-    let sys_file = unsafe { std::fs::File::from_raw_fd(file.as_raw_fd()) };
+
+    #[cfg(windows)]
+    let sys_file = unsafe { std::fs::File::from_raw_handle(file.as_raw_handle()) };
+    #[cfg(windows)]
     let size = sys_file.metadata()?.len() as usize;
-    let _ = sys_file.into_raw_fd();
+    #[cfg(windows)]
+    let _ = sys_file.into_raw_handle();
+
+    #[cfg(unix)]
+    let size = file.metadata().await?.len() as usize;
 
     let (res, buf) = file
         .read_exact_at(Vec::with_capacity(size).slice_mut(0..size), 0)

monoio/src/time/interval.rs

@@ -9,8 +9,9 @@ use crate::{
     time::{sleep_until, Duration, Instant, Sleep},
 };
 
-/// Creates new [`Interval`] that yields with interval of `period`. The first
-/// tick completes immediately. The default [`MissedTickBehavior`] is
+/// Creates new [`Interval`] that yields with interval of `period`.
+///
+/// The first tick completes immediately. The default [`MissedTickBehavior`] is
 /// [`Burst`](MissedTickBehavior::Burst), but this can be configured
 /// by calling [`set_missed_tick_behavior`](Interval::set_missed_tick_behavior).
 ///
@@ -79,9 +80,11 @@ pub fn interval(period: Duration) -> Interval {
 }
 
 /// Creates new [`Interval`] that yields with interval of `period` with the
-/// first tick completing at `start`. The default [`MissedTickBehavior`] is
-/// [`Burst`](MissedTickBehavior::Burst), but this can be configured
-/// by calling [`set_missed_tick_behavior`](Interval::set_missed_tick_behavior).
+/// first tick completing at `start`.
+///
+/// The default [`MissedTickBehavior`] is [`Burst`](MissedTickBehavior::Burst),
+/// but this can be configured by calling
+/// [`set_missed_tick_behavior`](Interval::set_missed_tick_behavior).
 ///
 /// An interval will tick indefinitely. At any time, the [`Interval`] value can
 /// be dropped. This cancels the interval.

monoio/tests/fs_file.rs

@@ -1,6 +1,4 @@
-// todo fix these CI in windows
-#![cfg(not(windows))]
-use std::io::prelude::*;
+use std::io::{prelude::*, SeekFrom};
 #[cfg(unix)]
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 #[cfg(windows)]
@@ -55,7 +53,7 @@ async fn basic_write() {
     file.write_at(HELLO, 0).await.0.unwrap();
     file.sync_all().await.unwrap();
 
-    let file = std::fs::read(tempfile.path()).unwrap();
+    let file = monoio::fs::read(tempfile.path()).await.unwrap();
     assert_eq!(file, HELLO);
 }
 
@@ -167,6 +165,7 @@ async fn poll_once(future: impl std::future::Future) {
     .await;
 }
 
+#[allow(unused, clippy::needless_return)]
 fn assert_invalid_fd(fd: RawFd, base: std::fs::Metadata) {
     use std::fs::File;
     #[cfg(unix)]
@@ -194,6 +193,7 @@ fn assert_invalid_fd(fd: RawFd, base: std::fs::Metadata) {
     }
 }
 
+#[cfg(unix)]
 #[monoio::test_all]
 async fn file_from_std() {
     let tempfile = tempfile();
@@ -208,3 +208,44 @@ async fn file_from_std() {
     file.sync_all().await.unwrap();
     read_hello(&file).await;
 }
+
+#[monoio::test_all]
+async fn position_read() {
+    let mut tempfile = tempfile();
+    tempfile.write_all(HELLO).unwrap();
+    tempfile.as_file_mut().sync_data().unwrap();
+
+    let file = File::open(tempfile.path()).await.unwrap();
+
+    // Modify the file pointer.
+    let mut std_file = std::fs::File::open(tempfile.path()).unwrap();
+    std_file.seek(SeekFrom::Start(8)).unwrap();
+
+    let buf = Vec::with_capacity(1024);
+    let (res, buf) = file.read_at(buf, 4).await;
+    let n = res.unwrap();
+
+    assert!(n > 0 && n <= HELLO.len() - 4);
+    assert_eq!(&buf, &HELLO[4..4 + n]);
+}
+
+#[monoio::test_all]
+async fn position_write() {
+    let tempfile = tempfile();
+
+    let file = File::create(tempfile.path()).await.unwrap();
+    file.write_at(HELLO, 0).await.0.unwrap();
+    file.sync_all().await.unwrap();
+
+    // Modify the file pointer.
+    let mut std_file = std::fs::File::open(tempfile.path()).unwrap();
+    std_file.seek(SeekFrom::Start(8)).unwrap();
+
+    file.write_at(b"monoio...", 6).await.0.unwrap();
+    file.sync_all().await.unwrap();
+
+    assert_eq!(
+        monoio::fs::read(tempfile.path()).await.unwrap(),
+        b"hello monoio..."
+    )
+}

monoio/tests/fs_read.rs

@@ -0,0 +1,19 @@
+use tempfile::NamedTempFile;
+
+const HELLO: &[u8] = b"hello world...";
+
+fn tempfile() -> NamedTempFile {
+    NamedTempFile::new().expect("unable to create tempfile")
+}
+
+#[monoio::test_all]
+async fn read_file_all() {
+    use std::io::Write;
+
+    let mut tempfile = tempfile();
+    tempfile.write_all(HELLO).unwrap();
+    tempfile.as_file_mut().sync_data().unwrap();
+
+    let res = monoio::fs::read(tempfile.path()).await.unwrap();
+    assert_eq!(res, HELLO);
+}