Align EnvPosix and EnvWindows.

Fixes #695.

PiperOrigin-RevId: 252895299

util/env_posix.cc

@@ -45,7 +45,7 @@ int g_open_read_only_file_limit = -1;
 // Up to 1000 mmap regions for 64-bit binaries; none for 32-bit.
 constexpr const int kDefaultMmapLimit = (sizeof(void*) >= 8) ? 1000 : 0;
 
-// Can be set using EnvPosixTestHelper::SetReadOnlyMMapLimit.
+// Can be set using EnvPosixTestHelper::SetReadOnlyMMapLimit().
 int g_mmap_limit = kDefaultMmapLimit;
 
 // Common flags defined for all posix open operations
@@ -491,7 +491,8 @@ class PosixEnv : public Env {
  public:
   PosixEnv();
   ~PosixEnv() override {
-    static char msg[] = "PosixEnv singleton destroyed. Unsupported behavior!\n";
+    static const char msg[] =
+        "PosixEnv singleton destroyed. Unsupported behavior!\n";
     std::fwrite(msg, 1, sizeof(msg), stderr);
     std::abort();
   }
@@ -663,7 +664,10 @@ class PosixEnv : public Env {
                 void* background_work_arg) override;
 
   void StartThread(void (*thread_main)(void* thread_main_arg),
-                   void* thread_main_arg) override;
+                   void* thread_main_arg) override {
+    std::thread new_thread(thread_main, thread_main_arg);
+    new_thread.detach();
+  }
 
   Status GetTestDirectory(std::string* result) override {
     const char* env = std::getenv("TEST_TMPDIR");
@@ -708,7 +712,9 @@ class PosixEnv : public Env {
     return static_cast<uint64_t>(tv.tv_sec) * kUsecondsPerSecond + tv.tv_usec;
   }
 
-  void SleepForMicroseconds(int micros) override { ::usleep(micros); }
+  void SleepForMicroseconds(int micros) override {
+    std::this_thread::sleep_for(std::chrono::microseconds(micros));
+  }
 
  private:
   void BackgroundThreadMain();
@@ -869,12 +875,6 @@ using PosixDefaultEnv = SingletonEnv;
 
 }  // namespace
 
-void PosixEnv::StartThread(void (*thread_main)(void* thread_main_arg),
-                           void* thread_main_arg) {
-  std::thread new_thread(thread_main, thread_main_arg);
-  new_thread.detach();
-}
-
 void EnvPosixTestHelper::SetReadOnlyFDLimit(int limit) {
   PosixDefaultEnv::AssertEnvNotInitialized();
   g_open_read_only_file_limit = limit;

util/env_windows.cc

@@ -13,9 +13,13 @@
 #include <atomic>
 #include <chrono>
 #include <condition_variable>
-#include <deque>
+#include <cstddef>
+#include <cstdint>
+#include <cstdlib>
+#include <cstring>
 #include <memory>
 #include <mutex>
+#include <queue>
 #include <sstream>
 #include <string>
 #include <vector>
@@ -40,9 +44,9 @@ namespace {
 constexpr const size_t kWritableFileBufferSize = 65536;
 
 // Up to 1000 mmaps for 64-bit binaries; none for 32-bit.
-constexpr int kDefaultMmapLimit = sizeof(void*) >= 8 ? 1000 : 0;
+constexpr int kDefaultMmapLimit = (sizeof(void*) >= 8) ? 1000 : 0;
 
-// Modified by EnvWindowsTestHelper::SetReadOnlyMMapLimit().
+// Can be set by by EnvWindowsTestHelper::SetReadOnlyMMapLimit().
 int g_mmap_limit = kDefaultMmapLimit;
 
 std::string GetWindowsErrorMessage(DWORD error_code) {
@@ -71,9 +75,12 @@ Status WindowsError(const std::string& context, DWORD error_code) {
 class ScopedHandle {
  public:
   ScopedHandle(HANDLE handle) : handle_(handle) {}
+  ScopedHandle(const ScopedHandle&) = delete;
   ScopedHandle(ScopedHandle&& other) noexcept : handle_(other.Release()) {}
   ~ScopedHandle() { Close(); }
 
+  ScopedHandle& operator=(const ScopedHandle&) = delete;
+
   ScopedHandle& operator=(ScopedHandle&& rhs) noexcept {
     if (this != &rhs) handle_ = rhs.Release();
     return *this;
@@ -142,44 +149,43 @@ class Limiter {
 
 class WindowsSequentialFile : public SequentialFile {
  public:
-  WindowsSequentialFile(std::string fname, ScopedHandle file)
-      : filename_(fname), file_(std::move(file)) {}
+  WindowsSequentialFile(std::string filename, ScopedHandle handle)
+      : handle_(std::move(handle)), filename_(std::move(filename)) {}
   ~WindowsSequentialFile() override {}
 
   Status Read(size_t n, Slice* result, char* scratch) override {
-    Status s;
     DWORD bytes_read;
     // DWORD is 32-bit, but size_t could technically be larger. However leveldb
     // files are limited to leveldb::Options::max_file_size which is clamped to
     // 1<<30 or 1 GiB.
     assert(n <= std::numeric_limits<DWORD>::max());
-    if (!::ReadFile(file_.get(), scratch, static_cast<DWORD>(n), &bytes_read,
+    if (!::ReadFile(handle_.get(), scratch, static_cast<DWORD>(n), &bytes_read,
                     nullptr)) {
-      s = WindowsError(filename_, ::GetLastError());
-    } else {
-      *result = Slice(scratch, bytes_read);
+      return WindowsError(filename_, ::GetLastError());
     }
-    return s;
+
+    *result = Slice(scratch, bytes_read);
+    return Status::OK();
   }
 
   Status Skip(uint64_t n) override {
     LARGE_INTEGER distance;
     distance.QuadPart = n;
-    if (!::SetFilePointerEx(file_.get(), distance, nullptr, FILE_CURRENT)) {
+    if (!::SetFilePointerEx(handle_.get(), distance, nullptr, FILE_CURRENT)) {
       return WindowsError(filename_, ::GetLastError());
     }
     return Status::OK();
   }
 
  private:
-  std::string filename_;
-  ScopedHandle file_;
+  const ScopedHandle handle_;
+  const std::string filename_;
 };
 
 class WindowsRandomAccessFile : public RandomAccessFile {
  public:
-  WindowsRandomAccessFile(std::string fname, ScopedHandle handle)
-      : filename_(fname), handle_(std::move(handle)) {}
+  WindowsRandomAccessFile(std::string filename, ScopedHandle handle)
+      : handle_(std::move(handle)), filename_(std::move(filename)) {}
 
   ~WindowsRandomAccessFile() override = default;
 
@@ -204,107 +210,116 @@ class WindowsRandomAccessFile : public RandomAccessFile {
   }
 
  private:
-  std::string filename_;
-  ScopedHandle handle_;
+  const ScopedHandle handle_;
+  const std::string filename_;
 };
 
 class WindowsMmapReadableFile : public RandomAccessFile {
  public:
   // base[0,length-1] contains the mmapped contents of the file.
-  WindowsMmapReadableFile(std::string fname, void* base, size_t length,
-                          Limiter* limiter)
-      : filename_(std::move(fname)),
-        mmapped_region_(base),
+  WindowsMmapReadableFile(std::string filename, char* mmap_base, size_t length,
+                          Limiter* mmap_limiter)
+      : mmap_base_(mmap_base),
         length_(length),
-        limiter_(limiter) {}
+        mmap_limiter_(mmap_limiter),
+        filename_(std::move(filename)) {}
 
   ~WindowsMmapReadableFile() override {
-    ::UnmapViewOfFile(mmapped_region_);
-    limiter_->Release();
+    ::UnmapViewOfFile(mmap_base_);
+    mmap_limiter_->Release();
   }
 
   Status Read(uint64_t offset, size_t n, Slice* result,
               char* scratch) const override {
-    Status s;
     if (offset + n > length_) {
       *result = Slice();
-      s = WindowsError(filename_, ERROR_INVALID_PARAMETER);
-    } else {
-      *result = Slice(reinterpret_cast<char*>(mmapped_region_) + offset, n);
+      return WindowsError(filename_, ERROR_INVALID_PARAMETER);
     }
-    return s;
+
+    *result = Slice(mmap_base_ + offset, n);
+    return Status::OK();
   }
 
  private:
-  std::string filename_;
-  void* mmapped_region_;
-  size_t length_;
-  Limiter* limiter_;
+  char* const mmap_base_;
+  const size_t length_;
+  Limiter* const mmap_limiter_;
+  const std::string filename_;
 };
 
 class WindowsWritableFile : public WritableFile {
  public:
-  WindowsWritableFile(std::string fname, ScopedHandle handle)
-      : filename_(std::move(fname)), handle_(std::move(handle)), pos_(0) {}
+  WindowsWritableFile(std::string filename, ScopedHandle handle)
+      : pos_(0), handle_(std::move(handle)), filename_(std::move(filename)) {}
 
   ~WindowsWritableFile() override = default;
 
   Status Append(const Slice& data) override {
-    size_t n = data.size();
-    const char* p = data.data();
+    size_t write_size = data.size();
+    const char* write_data = data.data();
 
     // Fit as much as possible into buffer.
-    size_t copy = std::min(n, kWritableFileBufferSize - pos_);
-    memcpy(buf_ + pos_, p, copy);
-    p += copy;
-    n -= copy;
-    pos_ += copy;
-    if (n == 0) {
+    size_t copy_size = std::min(write_size, kWritableFileBufferSize - pos_);
+    std::memcpy(buf_ + pos_, write_data, copy_size);
+    write_data += copy_size;
+    write_size -= copy_size;
+    pos_ += copy_size;
+    if (write_size == 0) {
       return Status::OK();
     }
 
     // Can't fit in buffer, so need to do at least one write.
-    Status s = FlushBuffered();
-    if (!s.ok()) {
-      return s;
+    Status status = FlushBuffer();
+    if (!status.ok()) {
+      return status;
     }
 
     // Small writes go to buffer, large writes are written directly.
-    if (n < kWritableFileBufferSize) {
-      memcpy(buf_, p, n);
-      pos_ = n;
+    if (write_size < kWritableFileBufferSize) {
+      std::memcpy(buf_, write_data, write_size);
+      pos_ = write_size;
       return Status::OK();
     }
-    return WriteRaw(p, n);
+    return WriteUnbuffered(write_data, write_size);
   }
 
   Status Close() override {
-    Status result = FlushBuffered();
-    if (!handle_.Close() && result.ok()) {
-      result = WindowsError(filename_, ::GetLastError());
+    Status status = FlushBuffer();
+    if (!handle_.Close() && status.ok()) {
+      status = WindowsError(filename_, ::GetLastError());
     }
-    return result;
+    return status;
   }
 
-  Status Flush() override { return FlushBuffered(); }
+  Status Flush() override { return FlushBuffer(); }
 
   Status Sync() override {
-    // On Windows no need to sync parent directory. It's metadata will be
-    // updated via the creation of the new file, without an explicit sync.
-    return FlushBuffered();
+    // On Windows no need to sync parent directory. Its metadata will be updated
+    // via the creation of the new file, without an explicit sync.
+
+    Status status = FlushBuffer();
+    if (!status.ok()) {
+      return status;
+    }
+
+    if (!::FlushFileBuffers(handle_.get())) {
+      return Status::IOError(filename_,
+                             GetWindowsErrorMessage(::GetLastError()));
+    }
+    return Status::OK();
   }
 
  private:
-  Status FlushBuffered() {
-    Status s = WriteRaw(buf_, pos_);
+  Status FlushBuffer() {
+    Status status = WriteUnbuffered(buf_, pos_);
     pos_ = 0;
-    return s;
+    return status;
   }
 
-  Status WriteRaw(const char* p, size_t n) {
+  Status WriteUnbuffered(const char* data, size_t size) {
     DWORD bytes_written;
-    if (!::WriteFile(handle_.get(), p, static_cast<DWORD>(n), &bytes_written,
-                     nullptr)) {
+    if (!::WriteFile(handle_.get(), data, static_cast<DWORD>(size),
+                     &bytes_written, nullptr)) {
       return Status::IOError(filename_,
                              GetWindowsErrorMessage(::GetLastError()));
     }
@@ -312,10 +327,11 @@ class WindowsWritableFile : public WritableFile {
   }
 
   // buf_[0, pos_-1] contains data to be written to handle_.
-  const std::string filename_;
-  ScopedHandle handle_;
   char buf_[kWritableFileBufferSize];
   size_t pos_;
+
+  ScopedHandle handle_;
+  const std::string filename_;
 };
 
 // Lock or unlock the entire file as specified by |lock|. Returns true
@@ -337,124 +353,132 @@ bool LockOrUnlock(HANDLE handle, bool lock) {
 
 class WindowsFileLock : public FileLock {
  public:
-  WindowsFileLock(ScopedHandle handle, std::string name)
-      : handle_(std::move(handle)), name_(std::move(name)) {}
+  WindowsFileLock(ScopedHandle handle, std::string filename)
+      : handle_(std::move(handle)), filename_(std::move(filename)) {}
 
-  ScopedHandle& handle() { return handle_; }
-  const std::string& name() const { return name_; }
+  const ScopedHandle& handle() const { return handle_; }
+  const std::string& filename() const { return filename_; }
 
  private:
-  ScopedHandle handle_;
-  std::string name_;
+  const ScopedHandle handle_;
+  const std::string filename_;
 };
 
 class WindowsEnv : public Env {
  public:
   WindowsEnv();
   ~WindowsEnv() override {
-    static char msg[] = "Destroying Env::Default()\n";
-    fwrite(msg, 1, sizeof(msg), stderr);
-    abort();
+    static const char msg[] =
+        "WindowsEnv singleton destroyed. Unsupported behavior!\n";
+    std::fwrite(msg, 1, sizeof(msg), stderr);
+    std::abort();
   }
 
-  Status NewSequentialFile(const std::string& fname,
+  Status NewSequentialFile(const std::string& filename,
                            SequentialFile** result) override {
     *result = nullptr;
     DWORD desired_access = GENERIC_READ;
     DWORD share_mode = FILE_SHARE_READ;
-    ScopedHandle handle =
-        ::CreateFileA(fname.c_str(), desired_access, share_mode, nullptr,
-                      OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
+    ScopedHandle handle = ::CreateFileA(
+        filename.c_str(), desired_access, share_mode,
+        /*lpSecurityAttributes=*/nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL,
+        /*hTemplateFile=*/nullptr);
     if (!handle.is_valid()) {
-      return WindowsError(fname, ::GetLastError());
+      return WindowsError(filename, ::GetLastError());
     }
-    *result = new WindowsSequentialFile(fname, std::move(handle));
+
+    *result = new WindowsSequentialFile(filename, std::move(handle));
     return Status::OK();
   }
 
-  Status NewRandomAccessFile(const std::string& fname,
+  Status NewRandomAccessFile(const std::string& filename,
                              RandomAccessFile** result) override {
     *result = nullptr;
     DWORD desired_access = GENERIC_READ;
     DWORD share_mode = FILE_SHARE_READ;
-    DWORD file_flags = FILE_ATTRIBUTE_READONLY;
-
     ScopedHandle handle =
-        ::CreateFileA(fname.c_str(), desired_access, share_mode, nullptr,
-                      OPEN_EXISTING, file_flags, nullptr);
+        ::CreateFileA(filename.c_str(), desired_access, share_mode,
+                      /*lpSecurityAttributes=*/nullptr, OPEN_EXISTING,
+                      FILE_ATTRIBUTE_READONLY,
+                      /*hTemplateFile=*/nullptr);
     if (!handle.is_valid()) {
-      return WindowsError(fname, ::GetLastError());
+      return WindowsError(filename, ::GetLastError());
     }
     if (!mmap_limiter_.Acquire()) {
-      *result = new WindowsRandomAccessFile(fname, std::move(handle));
+      *result = new WindowsRandomAccessFile(filename, std::move(handle));
       return Status::OK();
     }
 
     LARGE_INTEGER file_size;
+    Status status;
     if (!::GetFileSizeEx(handle.get(), &file_size)) {
-      return WindowsError(fname, ::GetLastError());
+      mmap_limiter_.Release();
+      return WindowsError(filename, ::GetLastError());
     }
 
     ScopedHandle mapping =
         ::CreateFileMappingA(handle.get(),
                              /*security attributes=*/nullptr, PAGE_READONLY,
                              /*dwMaximumSizeHigh=*/0,
-                             /*dwMaximumSizeLow=*/0, nullptr);
+                             /*dwMaximumSizeLow=*/0,
+                             /*lpName=*/nullptr);
     if (mapping.is_valid()) {
-      void* base = MapViewOfFile(mapping.get(), FILE_MAP_READ, 0, 0, 0);
-      if (base) {
+      void* mmap_base = ::MapViewOfFile(mapping.get(), FILE_MAP_READ,
+                                        /*dwFileOffsetHigh=*/0,
+                                        /*dwFileOffsetLow=*/0,
+                                        /*dwNumberOfBytesToMap=*/0);
+      if (mmap_base) {
         *result = new WindowsMmapReadableFile(
-            fname, base, static_cast<size_t>(file_size.QuadPart),
-            &mmap_limiter_);
+            filename, reinterpret_cast<char*>(mmap_base),
+            static_cast<size_t>(file_size.QuadPart), &mmap_limiter_);
         return Status::OK();
       }
     }
-    Status s = WindowsError(fname, ::GetLastError());
-
-    if (!s.ok()) {
-      mmap_limiter_.Release();
-    }
-    return s;
+    mmap_limiter_.Release();
+    return WindowsError(filename, ::GetLastError());
   }
 
-  Status NewWritableFile(const std::string& fname,
+  Status NewWritableFile(const std::string& filename,
                          WritableFile** result) override {
     DWORD desired_access = GENERIC_WRITE;
-    DWORD share_mode = 0;
-
-    ScopedHandle handle =
-        ::CreateFileA(fname.c_str(), desired_access, share_mode, nullptr,
-                      CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
+    DWORD share_mode = 0;  // Exclusive access.
+    ScopedHandle handle = ::CreateFileA(
+        filename.c_str(), desired_access, share_mode,
+        /*lpSecurityAttributes=*/nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL,
+        /*hTemplateFile=*/nullptr);
     if (!handle.is_valid()) {
       *result = nullptr;
-      return WindowsError(fname, ::GetLastError());
+      return WindowsError(filename, ::GetLastError());
     }
 
-    *result = new WindowsWritableFile(fname, std::move(handle));
+    *result = new WindowsWritableFile(filename, std::move(handle));
     return Status::OK();
   }
 
-  Status NewAppendableFile(const std::string& fname,
+  Status NewAppendableFile(const std::string& filename,
                            WritableFile** result) override {
-    ScopedHandle handle =
-        ::CreateFileA(fname.c_str(), FILE_APPEND_DATA, 0, nullptr, OPEN_ALWAYS,
-                      FILE_ATTRIBUTE_NORMAL, nullptr);
+    DWORD desired_access = FILE_APPEND_DATA;
+    DWORD share_mode = 0;  // Exclusive access.
+    ScopedHandle handle = ::CreateFileA(
+        filename.c_str(), desired_access, share_mode,
+        /*lpSecurityAttributes=*/nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL,
+        /*hTemplateFile=*/nullptr);
     if (!handle.is_valid()) {
       *result = nullptr;
-      return WindowsError(fname, ::GetLastError());
+      return WindowsError(filename, ::GetLastError());
     }
 
-    *result = new WindowsWritableFile(fname, std::move(handle));
+    *result = new WindowsWritableFile(filename, std::move(handle));
     return Status::OK();
   }
 
-  bool FileExists(const std::string& fname) override {
-    return GetFileAttributesA(fname.c_str()) != INVALID_FILE_ATTRIBUTES;
+  bool FileExists(const std::string& filename) override {
+    return GetFileAttributesA(filename.c_str()) != INVALID_FILE_ATTRIBUTES;
   }
 
-  Status GetChildren(const std::string& dir,
+  Status GetChildren(const std::string& directory_path,
                      std::vector<std::string>* result) override {
-    const std::string find_pattern = dir + "\\*";
+    const std::string find_pattern = directory_path + "\\*";
     WIN32_FIND_DATAA find_data;
     HANDLE dir_handle = ::FindFirstFileA(find_pattern.c_str(), &find_data);
     if (dir_handle == INVALID_HANDLE_VALUE) {
@@ -462,7 +486,7 @@ class WindowsEnv : public Env {
       if (last_error == ERROR_FILE_NOT_FOUND) {
         return Status::OK();
       }
-      return WindowsError(dir, last_error);
+      return WindowsError(directory_path, last_error);
     }
     do {
       char base_name[_MAX_FNAME];
@@ -476,105 +500,109 @@ class WindowsEnv : public Env {
     DWORD last_error = ::GetLastError();
     ::FindClose(dir_handle);
     if (last_error != ERROR_NO_MORE_FILES) {
-      return WindowsError(dir, last_error);
+      return WindowsError(directory_path, last_error);
     }
     return Status::OK();
   }
 
-  Status DeleteFile(const std::string& fname) override {
-    if (!::DeleteFileA(fname.c_str())) {
-      return WindowsError(fname, ::GetLastError());
+  Status DeleteFile(const std::string& filename) override {
+    if (!::DeleteFileA(filename.c_str())) {
+      return WindowsError(filename, ::GetLastError());
     }
     return Status::OK();
   }
 
-  Status CreateDir(const std::string& name) override {
-    if (!::CreateDirectoryA(name.c_str(), nullptr)) {
-      return WindowsError(name, ::GetLastError());
+  Status CreateDir(const std::string& dirname) override {
+    if (!::CreateDirectoryA(dirname.c_str(), nullptr)) {
+      return WindowsError(dirname, ::GetLastError());
     }
     return Status::OK();
   }
 
-  Status DeleteDir(const std::string& name) override {
-    if (!::RemoveDirectoryA(name.c_str())) {
-      return WindowsError(name, ::GetLastError());
+  Status DeleteDir(const std::string& dirname) override {
+    if (!::RemoveDirectoryA(dirname.c_str())) {
+      return WindowsError(dirname, ::GetLastError());
     }
     return Status::OK();
   }
 
-  Status GetFileSize(const std::string& fname, uint64_t* size) override {
-    WIN32_FILE_ATTRIBUTE_DATA attrs;
-    if (!::GetFileAttributesExA(fname.c_str(), GetFileExInfoStandard, &attrs)) {
-      return WindowsError(fname, ::GetLastError());
+  Status GetFileSize(const std::string& filename, uint64_t* size) override {
+    WIN32_FILE_ATTRIBUTE_DATA file_attributes;
+    if (!::GetFileAttributesExA(filename.c_str(), GetFileExInfoStandard,
+                                &file_attributes)) {
+      return WindowsError(filename, ::GetLastError());
     }
     ULARGE_INTEGER file_size;
-    file_size.HighPart = attrs.nFileSizeHigh;
-    file_size.LowPart = attrs.nFileSizeLow;
+    file_size.HighPart = file_attributes.nFileSizeHigh;
+    file_size.LowPart = file_attributes.nFileSizeLow;
     *size = file_size.QuadPart;
     return Status::OK();
   }
 
-  Status RenameFile(const std::string& src,
-                    const std::string& target) override {
-    // Try a simple move first.  It will only succeed when |to_path| doesn't
-    // already exist.
-    if (::MoveFileA(src.c_str(), target.c_str())) {
+  Status RenameFile(const std::string& from, const std::string& to) override {
+    // Try a simple move first. It will only succeed when |to| doesn't already
+    // exist.
+    if (::MoveFileA(from.c_str(), to.c_str())) {
       return Status::OK();
     }
     DWORD move_error = ::GetLastError();
 
     // Try the full-blown replace if the move fails, as ReplaceFile will only
-    // succeed when |to_path| does exist. When writing to a network share, we
-    // may not be able to change the ACLs. Ignore ACL errors then
+    // succeed when |to| does exist. When writing to a network share, we may not
+    // be able to change the ACLs. Ignore ACL errors then
     // (REPLACEFILE_IGNORE_MERGE_ERRORS).
-    if (::ReplaceFileA(target.c_str(), src.c_str(), nullptr,
-                       REPLACEFILE_IGNORE_MERGE_ERRORS, nullptr, nullptr)) {
+    if (::ReplaceFileA(to.c_str(), from.c_str(), /*lpBackupFileName=*/nullptr,
+                       REPLACEFILE_IGNORE_MERGE_ERRORS,
+                       /*lpExclude=*/nullptr, /*lpReserved=*/nullptr)) {
       return Status::OK();
     }
     DWORD replace_error = ::GetLastError();
-    // In the case of FILE_ERROR_NOT_FOUND from ReplaceFile, it is likely
-    // that |to_path| does not exist. In this case, the more relevant error
-    // comes from the call to MoveFile.
+    // In the case of FILE_ERROR_NOT_FOUND from ReplaceFile, it is likely that
+    // |to| does not exist. In this case, the more relevant error comes from the
+    // call to MoveFile.
     if (replace_error == ERROR_FILE_NOT_FOUND ||
         replace_error == ERROR_PATH_NOT_FOUND) {
-      return WindowsError(src, move_error);
+      return WindowsError(from, move_error);
     } else {
-      return WindowsError(src, replace_error);
+      return WindowsError(from, replace_error);
     }
   }
 
-  Status LockFile(const std::string& fname, FileLock** lock) override {
+  Status LockFile(const std::string& filename, FileLock** lock) override {
     *lock = nullptr;
     Status result;
     ScopedHandle handle = ::CreateFileA(
-        fname.c_str(), GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
+        filename.c_str(), GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
         /*lpSecurityAttributes=*/nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL,
         nullptr);
     if (!handle.is_valid()) {
-      result = WindowsError(fname, ::GetLastError());
+      result = WindowsError(filename, ::GetLastError());
     } else if (!LockOrUnlock(handle.get(), true)) {
-      result = WindowsError("lock " + fname, ::GetLastError());
+      result = WindowsError("lock " + filename, ::GetLastError());
     } else {
-      *lock = new WindowsFileLock(std::move(handle), std::move(fname));
+      *lock = new WindowsFileLock(std::move(handle), filename);
     }
     return result;
   }
 
   Status UnlockFile(FileLock* lock) override {
-    std::unique_ptr<WindowsFileLock> my_lock(
-        reinterpret_cast<WindowsFileLock*>(lock));
-    Status result;
-    if (!LockOrUnlock(my_lock->handle().get(), false)) {
-      result = WindowsError("unlock", ::GetLastError());
+    WindowsFileLock* windows_file_lock =
+        reinterpret_cast<WindowsFileLock*>(lock);
+    if (!LockOrUnlock(windows_file_lock->handle().get(), false)) {
+      return WindowsError("unlock " + windows_file_lock->filename(),
+                          ::GetLastError());
     }
-    return result;
+    delete windows_file_lock;
+    return Status::OK();
   }
 
-  void Schedule(void (*function)(void*), void* arg) override;
+  void Schedule(void (*background_work_function)(void* background_work_arg),
+                void* background_work_arg) override;
 
-  void StartThread(void (*function)(void* arg), void* arg) override {
-    std::thread t(function, arg);
-    t.detach();
+  void StartThread(void (*thread_main)(void* thread_main_arg),
+                   void* thread_main_arg) override {
+    std::thread new_thread(thread_main, thread_main_arg);
+    new_thread.detach();
   }
 
   Status GetTestDirectory(std::string* result) override {
@@ -601,7 +629,7 @@ class WindowsEnv : public Env {
     std::FILE* fp = std::fopen(filename.c_str(), "w");
     if (fp == nullptr) {
       *result = nullptr;
-      return WindowsError("NewLogger", ::GetLastError());
+      return WindowsError(filename, ::GetLastError());
     } else {
       *result = new WindowsLogger(fp);
       return Status::OK();
@@ -626,86 +654,147 @@ class WindowsEnv : public Env {
   }
 
  private:
-  // Entry per Schedule() call
-  struct BGItem {
-    void* arg;
-    void (*function)(void*);
+  void BackgroundThreadMain();
+
+  static void BackgroundThreadEntryPoint(WindowsEnv* env) {
+    env->BackgroundThreadMain();
+  }
+
+  // Stores the work item data in a Schedule() call.
+  //
+  // Instances are constructed on the thread calling Schedule() and used on the
+  // background thread.
+  //
+  // This structure is thread-safe beacuse it is immutable.
+  struct BackgroundWorkItem {
+    explicit BackgroundWorkItem(void (*function)(void* arg), void* arg)
+        : function(function), arg(arg) {}
+
+    void (*const function)(void*);
+    void* const arg;
   };
 
-  // BGThread() is the body of the background thread
-  void BGThread();
+  port::Mutex background_work_mutex_;
+  port::CondVar background_work_cv_ GUARDED_BY(background_work_mutex_);
+  bool started_background_thread_ GUARDED_BY(background_work_mutex_);
+
+  std::queue<BackgroundWorkItem> background_work_queue_
+      GUARDED_BY(background_work_mutex_);
 
-  std::mutex mu_;
-  std::condition_variable bgsignal_;
-  bool started_bgthread_;
-  std::deque<BGItem> queue_;
-  Limiter mmap_limiter_;
+  Limiter mmap_limiter_;  // Thread-safe.
 };
 
 // Return the maximum number of concurrent mmaps.
-int MaxMmaps() {
-  if (g_mmap_limit >= 0) {
-    return g_mmap_limit;
-  }
-  // Up to 1000 mmaps for 64-bit binaries; none for smaller pointer sizes.
-  g_mmap_limit = sizeof(void*) >= 8 ? 1000 : 0;
-  return g_mmap_limit;
-}
+int MaxMmaps() { return g_mmap_limit; }
 
 WindowsEnv::WindowsEnv()
-    : started_bgthread_(false), mmap_limiter_(MaxMmaps()) {}
+    : background_work_cv_(&background_work_mutex_),
+      started_background_thread_(false),
+      mmap_limiter_(MaxMmaps()) {}
 
-void WindowsEnv::Schedule(void (*function)(void*), void* arg) {
-  std::lock_guard<std::mutex> guard(mu_);
+void WindowsEnv::Schedule(
+    void (*background_work_function)(void* background_work_arg),
+    void* background_work_arg) {
+  background_work_mutex_.Lock();
 
-  // Start background thread if necessary
-  if (!started_bgthread_) {
-    started_bgthread_ = true;
-    std::thread t(&WindowsEnv::BGThread, this);
-    t.detach();
+  // Start the background thread, if we haven't done so already.
+  if (!started_background_thread_) {
+    started_background_thread_ = true;
+    std::thread background_thread(WindowsEnv::BackgroundThreadEntryPoint, this);
+    background_thread.detach();
   }
 
-  // If the queue is currently empty, the background thread may currently be
-  // waiting.
-  if (queue_.empty()) {
-    bgsignal_.notify_one();
+  // If the queue is empty, the background thread may be waiting for work.
+  if (background_work_queue_.empty()) {
+    background_work_cv_.Signal();
   }
 
-  // Add to priority queue
-  queue_.push_back(BGItem());
-  queue_.back().function = function;
-  queue_.back().arg = arg;
+  background_work_queue_.emplace(background_work_function, background_work_arg);
+  background_work_mutex_.Unlock();
 }
 
-void WindowsEnv::BGThread() {
+void WindowsEnv::BackgroundThreadMain() {
   while (true) {
-    // Wait until there is an item that is ready to run
-    std::unique_lock<std::mutex> lk(mu_);
-    bgsignal_.wait(lk, [this] { return !queue_.empty(); });
+    background_work_mutex_.Lock();
 
-    void (*function)(void*) = queue_.front().function;
-    void* arg = queue_.front().arg;
-    queue_.pop_front();
+    // Wait until there is work to be done.
+    while (background_work_queue_.empty()) {
+      background_work_cv_.Wait();
+    }
 
-    lk.unlock();
-    (*function)(arg);
+    assert(!background_work_queue_.empty());
+    auto background_work_function = background_work_queue_.front().function;
+    void* background_work_arg = background_work_queue_.front().arg;
+    background_work_queue_.pop();
+
+    background_work_mutex_.Unlock();
+    background_work_function(background_work_arg);
   }
 }
 
-}  // namespace
+// Wraps an Env instance whose destructor is never created.
+//
+// Intended usage:
+//   using PlatformSingletonEnv = SingletonEnv<PlatformEnv>;
+//   void ConfigurePosixEnv(int param) {
+//     PlatformSingletonEnv::AssertEnvNotInitialized();
+//     // set global configuration flags.
+//   }
+//   Env* Env::Default() {
+//     static PlatformSingletonEnv default_env;
+//     return default_env.env();
+//   }
+template <typename EnvType>
+class SingletonEnv {
+ public:
+  SingletonEnv() {
+#if !defined(NDEBUG)
+    env_initialized_.store(true, std::memory_order::memory_order_relaxed);
+#endif  // !defined(NDEBUG)
+    static_assert(sizeof(env_storage_) >= sizeof(EnvType),
+                  "env_storage_ will not fit the Env");
+    static_assert(alignof(decltype(env_storage_)) >= alignof(EnvType),
+                  "env_storage_ does not meet the Env's alignment needs");
+    new (&env_storage_) EnvType();
+  }
+  ~SingletonEnv() = default;
+
+  SingletonEnv(const SingletonEnv&) = delete;
+  SingletonEnv& operator=(const SingletonEnv&) = delete;
+
+  Env* env() { return reinterpret_cast<Env*>(&env_storage_); }
 
-static std::once_flag once;
-static Env* default_env;
-static void InitDefaultEnv() { default_env = new WindowsEnv(); }
+  static void AssertEnvNotInitialized() {
+#if !defined(NDEBUG)
+    assert(!env_initialized_.load(std::memory_order::memory_order_relaxed));
+#endif  // !defined(NDEBUG)
+  }
+
+ private:
+  typename std::aligned_storage<sizeof(EnvType), alignof(EnvType)>::type
+      env_storage_;
+#if !defined(NDEBUG)
+  static std::atomic<bool> env_initialized_;
+#endif  // !defined(NDEBUG)
+};
+
+#if !defined(NDEBUG)
+template <typename EnvType>
+std::atomic<bool> SingletonEnv<EnvType>::env_initialized_;
+#endif  // !defined(NDEBUG)
+
+using WindowsDefaultEnv = SingletonEnv<WindowsEnv>;
+
+}  // namespace
 
 void EnvWindowsTestHelper::SetReadOnlyMMapLimit(int limit) {
-  assert(default_env == nullptr);
+  WindowsDefaultEnv::AssertEnvNotInitialized();
   g_mmap_limit = limit;
 }
 
 Env* Env::Default() {
-  std::call_once(once, InitDefaultEnv);
-  return default_env;
+  static WindowsDefaultEnv env_container;
+  return env_container.env();
 }
 
 }  // namespace leveldb