Exception unification improvements #5911

tgoyne · 2022-09-29T15:09:15Z

Most of these changes are related to improving the messages in exceptions thrown by core so that we can pass them on directly to the user rather than having to replace them with better errors in the SDK. With these changes, there's only a small number of places where Cocoa modifies the messages thrown by core, and they're all clearly linked to Cocoa-specific quirks.

This also greatly expands the testing of the exceptions thrown by core, as there were a number of places where Cocoa tests identified errors with messages that were simply wrong. Every instance of REQUIRE_THROWS() in the object store tests has been replaced with a more precise assertion which validations that the expected thing is being thrown; in most cases with REQUIRE_EXCEPTION(), which is a new test macro which checks that the exception is a realm::Exception, has the expected error code, and validates the exception message (either using a literal string or a Catch matcher). This revealed a few places where tests weren't testing the thing they intended to test, many places where things other than realm::Exception were being thrown, and a lot of spots where error messages could just be better.

The binary size increased by quite a bit with the exception unification branch, so I made some localized changes to address this. Giving each of the exception types a non-inline virtual function makes it so that the vtables for the exception types aren't inline and thus aren't emitted into every since TU which includes the header (which is most of them). This cuts about a megabyte of a static library build. The std::map for converting error code names to codes also had surprisingly bad codegen due to building the map at runtime, and replacing it with a statically sorted array both reduces the size and improves startup perf.

bmunkholm · 2022-09-30T16:13:37Z

@tgoyne How much bigger is the binary now with this PR?

tgoyne · 2022-09-30T23:09:25Z

Current sizes of Realm.framework:

branch	static	dynamic
master	114,406 KB	10,325 KB
feature/exception-unification	115,499 KB	10,374 KB
tg/exception-unification	114,877 KB	10,356 KB

tgoyne · 2022-10-20T23:16:01Z

src/realm/alloc_slab.cpp

- // Create a deep copy of the file_path string, otherwise it can appear that
- // users are leaking paths because string assignment operator implementations might
- // actually be reference counting with copy-on-write. If our all_files map
- // holds onto these references (since it is still reachable memory) it can appear
- // as a leak in the user application, but it is actually us (and that's ok).
- const std::string path = file_path.c_str();


This was a workaround for a c++03 thing (copy-on-write std::string) which hasn't been applicable since c++11.

tgoyne · 2022-10-20T23:18:57Z

src/realm/exceptions.cpp

@@ -85,31 +85,35 @@ UnsupportedFileFormatVersion::UnsupportedFileFormatVersion(int version)
 UnsupportedFileFormatVersion::~UnsupportedFileFormatVersion() noexcept = default;


-LogicError::LogicError(ErrorCodes::Error code, const std::string& msg)
+LogicError::LogicError(ErrorCodes::Error code, std::string_view msg)


There's one or two spots where being able to pass in a string_view is nice, and that ends up being a viral change that needs to propagate all the way downwards so it touches all of these.

tgoyne · 2022-10-20T23:23:56Z

src/realm/object-store/impl/apple/external_commit_helper.cpp

@@ -122,11 +122,11 @@ ExternalCommitHelper::ExternalCommitHelper(RealmCoordinator& parent, const Realm
 std::string temp_dir = util::normalize_dir(config.fifo_files_fallback_path);
 std::string sys_temp_dir = util::normalize_dir(DBOptions::get_sys_tmp_dir());
 path = DB::get_core_file(config.path, DB::CoreFileType::Note);
- bool fifo_created = realm::util::try_create_fifo(path);
+ bool fifo_created = realm::util::try_create_fifo(path, !temp_dir.empty() || !sys_temp_dir.empty());


The change here is making it only swallow the exception if we have more paths to try, which results in a more useful error when we can't create the fifo and don't have a fallback.

tgoyne · 2022-10-20T23:24:19Z

src/realm/object-store/impl/realm_coordinator.cpp

@@ -425,31 +425,6 @@ void RealmCoordinator::open_db()
 util::File::remove(m_config.path);
 return open_db();
 }
- catch (const FileAccessError& ex) {


This now is all handled in File::open()

tgoyne · 2022-10-20T23:26:26Z

src/realm/object-store/sync/sync_session.cpp

@@ -154,7 +154,7 @@ void SyncSession::become_inactive(util::CheckedUniqueLock lock, Status status)
 }

 if (!status.get_std_error_code())
- status = Status(make_error_code(util::error::operation_aborted), "SyncSession::become_inactive");
+ status = Status(make_error_code(util::error::operation_aborted), "Sync session became inactive");


This error is still not great but correctly reporting why the listener was cancelled turned out to be oddly complicated.

tgoyne · 2022-10-20T23:52:28Z

src/realm/util/file.cpp

 default:
 throw FileAccessError(ErrorCodes::FileOperationFailed, msg, path, err); // LCOV_EXCL_LINE
 }
 }


-void remove_dir_recursive(const std::string& path)


This wasn't actually used.

tgoyne · 2022-10-21T00:00:58Z

test/object-store/collection_fixtures.hpp


 template <typename T>
 inline T get(Mixed)
 {
- abort();


The main goal of these changes is to eliminate these aborts and replace them with compile-time failures. As a bonus it ends up less verbose and actually compilers faster due to eliminating unreachable template instantiations.

jedelbo

LGTM

This turns some runtime failures for misuse into compile time failures and improves the compilation time of the tests a bit by eliminating a bunch of unreachable template instantiations.

…o the server

If there was no temporary directory available and creating the fifo at the default path failed, it reported an error for trying to open the nonexistent fifo rather than creating the fifo failing.

tgoyne self-assigned this Sep 29, 2022

cla-bot bot added the cla: yes label Sep 29, 2022

tgoyne force-pushed the tg/exception-unification branch from 8dba10d to e665c4d Compare September 30, 2022 00:06

tgoyne force-pushed the tg/exception-unification branch 2 times, most recently from 7f33825 to a2287ce Compare October 5, 2022 01:24

jedelbo mentioned this pull request Oct 7, 2022

Je/exception unification #5927

Merged

tgoyne force-pushed the tg/exception-unification branch from a2287ce to 37f40e3 Compare October 8, 2022 00:05

tgoyne changed the base branch from feature/exception-unification to je/exception-unification October 8, 2022 00:06

tgoyne force-pushed the tg/exception-unification branch 10 times, most recently from 4c3fddb to fb7a3a1 Compare October 10, 2022 01:50

Base automatically changed from je/exception-unification to feature/exception-unification October 10, 2022 09:18

This was referenced Oct 10, 2022

Je/exception unification #5930

Merged

Je/exception unification #5934

Closed

tgoyne force-pushed the tg/exception-unification branch 6 times, most recently from ac0df23 to ccc8a1d Compare October 20, 2022 23:15

tgoyne commented Oct 21, 2022

View reviewed changes

tgoyne force-pushed the tg/exception-unification branch 2 times, most recently from 0b2171b to 6789384 Compare October 21, 2022 01:18

tgoyne marked this pull request as ready for review October 21, 2022 02:35

tgoyne mentioned this pull request Oct 24, 2022

Adopt exception unification realm/realm-swift#8002

Merged

jedelbo approved these changes Oct 26, 2022

View reviewed changes

tgoyne force-pushed the tg/exception-unification branch from 6789384 to 0b00657 Compare October 26, 2022 18:12

tgoyne added 12 commits October 26, 2022 11:25

Fix some formatting errors introduced in merges

b8e5b7b

Expose the error category for basic_system_error

6bdcd7a

constexprify the collection fixtures

ae7b2b6

This turns some runtime failures for misuse into compile time failures and improves the compilation time of the tests a bit by eliminating a bunch of unreachable template instantiations.

Make more StringData operations constexpr

a220b8b

Delete some unused test helper code

28a2b59

Convert some uses of std exceptions to realm::Exception

65d1d8e

Add missing validation to many of Realm's entry point functions

9c3f5fc

Remove the baas dependency for some tests which don't actually talk t…

75cd8e3

…o the server

Improve error reporting when initializing ExternalCommitHelper fails

c995aa6

If there was no temporary directory available and creating the fifo at the default path failed, it reported an error for trying to open the nonexistent fifo rather than creating the fifo failing.

Improve the error messages in exceptions thrown by collections

74e3d8d

Improve error messages for file operations and opening Realm files

9eb5580

Validate the message and code for most tests which test exceptions

6e06cc6

tgoyne force-pushed the tg/exception-unification branch from 0b00657 to 6e06cc6 Compare October 26, 2022 18:35

tgoyne merged commit 506f118 into feature/exception-unification Oct 26, 2022

tgoyne deleted the tg/exception-unification branch October 26, 2022 19:40

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

Exception unification improvements #5911

Exception unification improvements #5911

tgoyne commented Sep 29, 2022

bmunkholm commented Sep 30, 2022

tgoyne commented Sep 30, 2022

tgoyne Oct 20, 2022

tgoyne Oct 20, 2022

tgoyne Oct 20, 2022

tgoyne Oct 20, 2022

tgoyne Oct 20, 2022

tgoyne Oct 20, 2022

tgoyne Oct 21, 2022

jedelbo left a comment

Exception unification improvements #5911

Exception unification improvements #5911

Conversation

tgoyne commented Sep 29, 2022

bmunkholm commented Sep 30, 2022

tgoyne commented Sep 30, 2022

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 20, 2022

Choose a reason for hiding this comment

tgoyne Oct 21, 2022

Choose a reason for hiding this comment

jedelbo left a comment

Choose a reason for hiding this comment