From 2af1fcb1ff9852d7878a672b8c2d4ad76b223a74 Mon Sep 17 00:00:00 2001
From: Jeremy Braun <jtbraun@meta.com>
Date: Thu, 29 Aug 2024 14:52:48 -0700
Subject: [PATCH] handle cases where ReadFile/WriteFile may complete
 synchronously (#161)

Summary:
Pull Request resolved: https://github.com/facebookresearch/vrs/pull/161

Unlike `ReadFileEx` and `WriteFileEx`, which always call their completion routines and are always asynchrous, `ReadFile` and `WriteFile` may complete synchronously. [This MSDN article](https://learn.microsoft.com/en-us/previous-versions/troubleshoot/windows/win32/asynchronous-disk-io-synchronous) discusses how to handle this (which is different from some of the advice given in ReadFile's documentation. That says
> The lpNumberOfBytesRead parameter should be set to NULL. Use the GetOverlappedResult function to get the actual number of bytes read.

If you do that, there's no way to know how many bytes were transferred if the operation completes synchronously.

Reviewed By: georges-berenger

Differential Revision: D61977068

fbshipit-source-id: cb7c4c6e3b86ff4b51ac29e6ec38fd9d382ef14f
---
 vrs/AsyncDiskFileChunk.hpp                  | 32 ++++++++++-----------
 vrs/test/file_tests/DeviceSimulatorTest.cpp |  3 --
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/vrs/AsyncDiskFileChunk.hpp b/vrs/AsyncDiskFileChunk.hpp
index 6f03f8a7..b151692e 100644
--- a/vrs/AsyncDiskFileChunk.hpp
+++ b/vrs/AsyncDiskFileChunk.hpp
@@ -172,30 +172,30 @@ struct AsyncWindowsHandle {
       return readNotWrite ? DISKFILE_NOT_ENOUGH_DATA : DISKFILE_PARTIAL_WRITE_ERROR;
     }
 
-    OVERLAPPED ov;
-
-    ov.hEvent = 0;
+    // N.B. this does not create an hEvent for the OVERLAPPED structure, instead using the file
+    // handle. This is only a valid thing to do if there are NO other IO operations occuring during
+    // this one. The calls to flushWriteBuffer() before calling this ensures this is the case.
+    OVERLAPPED ov = {};
     ov.Offset = (DWORD)offset;
     ov.OffsetHigh = (DWORD)(offset >> 32);
 
+    DWORD dwNumberOfBytesTransferred = 0;
+    bool success = false;
     if (readNotWrite) {
-      if (ReadFile(h_, buf, dwToXfer, nullptr, &ov)) {
-        return SUCCESS;
-      }
+      success = ReadFile(h_, buf, dwToXfer, &dwNumberOfBytesTransferred, &ov);
     } else {
-      if (WriteFile(h_, buf, dwToXfer, nullptr, &ov)) {
-        return SUCCESS;
-      }
+      success = WriteFile(h_, buf, dwToXfer, &dwNumberOfBytesTransferred, &ov);
     }
 
-    int error = GetLastError();
-    if (error != ERROR_IO_PENDING) {
-      return error;
-    }
+    if (!success) {
+      int error = GetLastError();
+      if (error != ERROR_IO_PENDING) {
+        return error;
+      }
 
-    DWORD dwNumberOfBytesTransferred;
-    if (!GetOverlappedResult(h_, &ov, &dwNumberOfBytesTransferred, TRUE)) {
-      return GetLastError();
+      if (!GetOverlappedResult(h_, &ov, &dwNumberOfBytesTransferred, TRUE)) {
+        return GetLastError();
+      }
     }
 
     outSize = dwNumberOfBytesTransferred;
diff --git a/vrs/test/file_tests/DeviceSimulatorTest.cpp b/vrs/test/file_tests/DeviceSimulatorTest.cpp
index b99bc341..9091fb63 100644
--- a/vrs/test/file_tests/DeviceSimulatorTest.cpp
+++ b/vrs/test/file_tests/DeviceSimulatorTest.cpp
@@ -25,7 +25,6 @@
 
 #include <vrs/DiskFile.h>
 #include <vrs/RecordFileReader.h>
-#include <vrs/os/Platform.h>
 #include <vrs/os/Utils.h>
 
 #include <vrs/test/helpers/VRSTestsHelpers.h>
@@ -128,7 +127,6 @@ TEST_F(DeviceSimulator, multiThreadAsyncAioNotDirect) {
   deleteChunkedFile(testPath);
 }
 
-#if IS_VRS_FB_INTERNAL() || !IS_WINDOWS_PLATFORM() // avoid OSS/Windows
 TEST_F(DeviceSimulator, multiThreadAsyncSync) {
   const string testPath = os::getTempFolder() + "MultiThreadAsyncSync.vrs";
 
@@ -140,7 +138,6 @@ TEST_F(DeviceSimulator, multiThreadAsyncSync) {
 
   deleteChunkedFile(testPath);
 }
-#endif
 
 TEST_F(DeviceSimulator, multiThreadAsyncPsync) {
   const string testPath = os::getTempFolder() + "MultiThreadAsyncPsync.vrs";