Improve util/file for Windows #6221

fealebenpae · 2023-01-21T03:09:15Z

What, How & Why?

While looking at the Emscripten filesystem implementation I saw that there are a bunch of Windows-specific code paths in our file handling that can be improved. Since on Windows we always have std::filesystem available it's better to replace the Win32-specific implementations with ones based on std::filesystem. That way instead of a mix of Win32 and C APIs that require manual conversion between char and wchar strings for paths we can use the simpler std::filesystem implementation.

This also implements File::get_unique_id for Windows and refactors existing APIs and tests to use that instead of doing it manually.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

fealebenpae · 2023-01-21T17:59:37Z

src/realm/db.cpp

-#ifdef _WIN32
-        // can't rename to existing file on Windows
-        util::File::copy(tmp_path, m_db_path);
-        util::File::remove(tmp_path);
-#else
        util::File::move(tmp_path, m_db_path);
-#endif


File::move() is now implemented in terms of std::filesystem::rename(), which does support overwriting renames on Windows.

jedelbo

LGTM

RedBeard0531 · 2023-02-03T09:44:28Z

src/realm/util/file.hpp

 #ifndef _WIN32
 #include <dirent.h> // POSIX.1-2001
+#else
+#include <Windows.h>
 #endif


Nit: would you mind switching to an #ifdef and flipping the cases? I have a pet peeve for negated ifs with an else case, since it is basically saying "if non not windows".

RedBeard0531 · 2023-02-03T09:48:45Z

src/realm/util/file.hpp

@@ -1359,7 +1357,8 @@ inline File::Exists::Exists(const std::string& msg, const std::string& path)
 inline bool operator==(const File::UniqueID& lhs, const File::UniqueID& rhs)
 {
 #ifdef _WIN32 // Windows version
-    throw util::runtime_error("Not yet supported");
+    return lhs.id_info.VolumeSerialNumber == rhs.id_info.VolumeSerialNumber &&
+           memcmp(&lhs.id_info.FileId, &rhs.id_info.FileId, sizeof(FILE_ID_128)) == 0;


Suggested change

memcmp(&lhs.id_info.FileId, &rhs.id_info.FileId, sizeof(FILE_ID_128)) == 0;

memcmp(&lhs.id_info.FileId, &rhs.id_info.FileId, sizeof(lhs.id_info.FileId)) == 0;

It is usually preferable to use one of the expressions in the memfoo sizeof rather than a fixed type, that way you are guaranteed to use the right type. That is unless the expressions have some weird incorrect type and you actually do need to explicitly specify a different type.

RedBeard0531 · 2023-02-03T09:52:08Z

src/realm/util/file.hpp

@@ -1373,7 +1372,10 @@ inline bool operator!=(const File::UniqueID& lhs, const File::UniqueID& rhs)
 inline bool operator<(const File::UniqueID& lhs, const File::UniqueID& rhs)
 {
 #ifdef _WIN32 // Windows version
-    throw util::runtime_error("Not yet supported");
+    return lhs.id_info.VolumeSerialNumber < rhs.id_info.VolumeSerialNumber &&
+           std::lexicographical_compare(


Please avoid std::lexicographical_compare and use memcmp() < 0 instead. You could also make std::string_views and use < on them if you would prefer. One of the problems is that std::lexicographical_compare uses a different, incorrect, and slower ordering of char.

src/realm/util/file.hpp

RedBeard0531 · 2023-02-03T09:56:18Z

test/object-store/migrations.cpp

+        REQUIRE(util::File::get_unique_id(config.path, id));
+        return id;


Could this just be return util::File::get_unique_id(config.path)?

We don't have an overload of UniqueID File::get_unique_id(std::string).

Yup, I misread that file. It would be nice to have though 😄

But totally fine if you don't want to do it right now.

How about transforming bool get_unique_id(const std::string&, UniqueID&) into std::optional<UniqueID> get_unique_id(const std::string& path)? I think that would make using it less clumsy.

RedBeard0531 · 2023-02-03T10:05:30Z

src/realm/util/file.cpp

+    if (error == errc::permission_denied || error == errc::read_only_file_system ||
+        error == errc::device_or_resource_busy || error == errc::operation_not_permitted ||
+        error == errc::file_exists || error == errc::directory_not_empty) {
+        throw File::PermissionDenied(error.message(), path);


It feels weird that we have different definitions of "permission denied" here than on line 158. It seems like we should be able to have a throwFileExceptionIfFailed(std::error_code) function to handle this. If you do that, it could replace the if (!error) on line 237 as well, so you could just do throwIfFailed(error); return result;

Edit: I see that there are a ton of blocks like this. Unless they really need different checks, please pull this out to a function.

Very good point. I've refactored these blocks out to a helper function.

RedBeard0531 · 2023-02-03T10:14:37Z

src/realm/util/file.cpp

@@ -1564,7 +1569,19 @@ FileDesc File::get_descriptor() const
 bool File::get_unique_id(const std::string& path, File::UniqueID& uid)
 {
 #ifdef _WIN32 // Windows version
-    throw std::runtime_error("Not yet supported");
+    WindowsFileHandleHolder fileHandle(::CreateFile2(std::filesystem::path(path).c_str(), FILE_READ_ATTRIBUTES,


Ugh, why would windows have you call a function named CreateFile if it don't want to actually create a file. Would you mind adding a comment like // Despite the name, this call won't actually create a file if it doesn't exist yet.

RedBeard0531 · 2023-02-03T10:19:34Z

src/realm/util/file.cpp

+        return path;
+
+    return (std::filesystem::path(base_dir) / path_).string();
+#elif !defined(_WIN32)


Isn't this always true now, so that the final #else is dead code?

RedBeard0531 · 2023-02-03T10:25:26Z

src/realm/util/file.cpp

+    if (path_.is_absolute())
+        return path;


Isn't this included in the behavior of path's operator<? If this is different and needed for some reason, a comment explaining why would be nice.

You're absolutely right. According to https://github.com/microsoft/STL/blob/73924c1920af92899f7582cd904ea819b9db35bc/stl/inc/filesystem#L708-L720 that's indeed the case. Side note: I love that Microsoft's STL is open source now and I can get useful information about its workings like this.

FYI, I just looked it up at https://en.cppreference.com/w/cpp/filesystem/path/append (operator/ docs just say to see here for behavior)

src/realm/util/file.cpp

…lm-core into yg/win32-file-uniqueid

Co-authored-by: Mathias Stearn <[email protected]>

This reverts commit 6b0bb2f.

…le-uniqueid # Conflicts: # CHANGELOG.md

…le-uniqueid

RedBeard0531

Sorry, forgot to publish comments after doing the review!

src/realm/util/file.cpp

src/realm/util/file.hpp

Co-authored-by: Mathias Stearn <[email protected]>

Improve util/file for Windows

c06ccf5

cla-bot bot added the cla: yes label Jan 21, 2023

github-actions bot assigned fealebenpae Jan 21, 2023

fealebenpae added 2 commits January 21, 2023 17:58

changes

b8f60b4

update basic_system error tests

12602a9

fealebenpae commented Jan 21, 2023

View reviewed changes

fealebenpae added 2 commits January 21, 2023 19:32

File::get_free_space now using std::filesystem

42e4f74

changelog

727302b

fealebenpae requested review from RedBeard0531 and jedelbo January 21, 2023 18:35

fealebenpae marked this pull request as ready for review January 21, 2023 18:53

fealebenpae and others added 3 commits January 21, 2023 20:18

fix tests

edcf7f4

Merge branch 'master' into yg/win32-file-uniqueid

72699b6

bring back useful comment

24a832b

jedelbo approved these changes Feb 3, 2023

View reviewed changes

RedBeard0531 reviewed Feb 3, 2023

View reviewed changes

fealebenpae and others added 12 commits February 3, 2023 18:46

address code review comments

9c080b8

Merge branch 'yg/win32-file-uniqueid' of https://github.com/realm/rea…

8c054d4

…lm-core into yg/win32-file-uniqueid

Update src/realm/util/file.cpp

07dc7de

Co-authored-by: Mathias Stearn <[email protected]>

Merge branch 'master' into yg/win32-file-uniqueid

1d7d1cd

fix formatting

1b7af91

fix tests

f9f62b8

remove File::UniqueID comparison operators

6b0bb2f

Revert "remove File::UniqueID comparison operators"

e6b89cf

This reverts commit 6b0bb2f.

fix File::UniqueID operator< on Windows

b247a6a

Merge branch 'master' of github.com:realm/realm-core into yg/win32-fi…

790bc6e

…le-uniqueid # Conflicts: # CHANGELOG.md

Merge branch 'master' into yg/win32-file-uniqueid

9cd7102

Merge branch 'master' of github.com:realm/realm-core into yg/win32-fi…

96dc052

…le-uniqueid

RedBeard0531 approved these changes Feb 15, 2023

View reviewed changes

src/realm/util/file.cpp Outdated Show resolved Hide resolved

src/realm/util/file.cpp Outdated Show resolved Hide resolved

src/realm/util/file.hpp Outdated Show resolved Hide resolved

Apply suggestions from code review

fb8b2e6

Co-authored-by: Mathias Stearn <[email protected]>

fealebenpae merged commit 18c99b7 into master Feb 15, 2023

fealebenpae deleted the yg/win32-file-uniqueid branch February 15, 2023 13:02

fealebenpae mentioned this pull request Feb 24, 2023

Regression with Core 13.5.0 when opening a non-ascii file on Windows #6336

Closed

kiburtse mentioned this pull request Mar 3, 2023

Merge master, adapt exceptions #6361

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve util/file for Windows #6221

Improve util/file for Windows #6221

fealebenpae commented Jan 21, 2023 •

edited

Loading

fealebenpae Jan 21, 2023

jedelbo left a comment

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

fealebenpae Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

fealebenpae Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

fealebenpae Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 Feb 3, 2023

fealebenpae Feb 3, 2023

RedBeard0531 Feb 3, 2023

RedBeard0531 left a comment

	memcmp(&lhs.id_info.FileId, &rhs.id_info.FileId, sizeof(FILE_ID_128)) == 0;
	memcmp(&lhs.id_info.FileId, &rhs.id_info.FileId, sizeof(lhs.id_info.FileId)) == 0;

		REQUIRE(util::File::get_unique_id(config.path, id));
		return id;

Improve util/file for Windows #6221

Improve util/file for Windows #6221

Conversation

fealebenpae commented Jan 21, 2023 • edited Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

jedelbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 left a comment

Choose a reason for hiding this comment

fealebenpae commented Jan 21, 2023 •

edited

Loading