Check if the Realm folder exists before using the lock file.

[DominicFrei]

When deleting a Realm we catch a File::NotFound exception in case the folder does not exist, see https://github.com/realm/realm-core/blob/master/src/realm/object-store/shared_realm.cpp#L917-L923 (added with #4771).

Unfortunately this check is not sufficient on Windows and crashes when trying to access the lock file (see https://ci.realm.io/blue/organizations/jenkins/realm%2Frealm-dotnet/detail/PR-2558/1/tests for example).

It couldn't have been caught earlier since the OS tests are not run on Windows. It only surfaced when run through the .NET CI with a similar check on the SDK level (see https://github.com/realm/realm-dotnet/blob/master/Tests/Realm.Tests/Database/InstanceTests.cs#L96-L105).

P.S.: This check was originally added with #4765.

☑️ ToDos

• 📝 Changelog update
• 🚦 Tests (or not relevant) -> There are no relevant test changes for my code changes in Core because of above mentioned reasons (it failed already, but only on Windows). The above mentioned .NET test serves as the relevant test for this change until the Core CI runs on Windows as well.
  I did, however, add a check for the did_delete flag to the non-existing folder test (which is unrelated to the code changes though).

[tgoyne]

Checking if a file exists before trying to do something with it is the canonical example of a TOCTOU race: just because File::exists() returns true doesn't mean that the file will still exist when we call DB::call_with_lock().

If some exception other than File::NotFound is being thrown here on Windows, we should simply be catching that exception too (or fixing the thing throwing the exception so that it throws the correct one).

[DominicFrei]

Checking if a file exists before trying to do something with it is the canonical example of a TOCTOU race: just because File::exists() returns true doesn't mean that the file will still exist when we call DB::call_with_lock().

If some exception other than File::NotFound is being thrown here on Windows, we should simply be catching that exception too (or fixing the thing throwing the exception so that it throws the correct one).

Thank you for this suggestion, it led me to some further investigations and the solution is not completely different from what I expected.

As it turns out the problem actually lies within the differing Windows and POSIX implementations for File::open_internal and their error handling.

Since File::NotFound is described as Thrown if the directory part of the specified path was not found I have added the ERROR_PATH_NOT_FOUND to also be thrown as a FileNotFound instead of the more generic AccessError which satisfies all requirements. 👍

[ironage]

Can you add a test similar to https://github.com/realm/realm-core/blob/master/test/test_file.cpp#L387-L392 but with part of the path missing instead of the file itself? The core and sync tests are run on windows CI, just not the object-store tests yet.

[DominicFrei]

Can you add a test similar to https://github.com/realm/realm-core/blob/master/test/test_file.cpp#L387-L392 but with part of the path missing instead of the file itself? The core and sync tests are run on windows CI, just not the object-store tests yet.

Thanks for pointing that out. I've added a test and will let it run (temporarily commented out the fix) to see if CI catches it.

[DominicFrei]

Run through CI (for the Windows part) everything's looking fine. The test was failing in https://spruce.mongodb.com/task/realm_core_stable_windows_64_vs2019_core_tests_patch_bc53af903b74c553308c91227ddfa29e6c6705c1_6126302632f417350263318a_21_08_25_11_57_27/tests?execution=0&sortBy=STATUS&sortDir=ASC and then successful in https://spruce.mongodb.com/version/612639752fbabe6e534cb6c5/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&variant=windows-64-vs2019 when re-enabling the fix (I couldn't run it locally that's why I over-complicated it via CI to make sure everything's fine).

PR is ready again. 👍

[ironage]

Thanks for adding the test and making sure it fails without the fix. 👍

[ironage]

I noticed that Jenkins CI didn't fail when it should have, and #4874 should fix that.