Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() #6667

Merged
merged 4 commits into from
May 26, 2023
Merged

Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() #6667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c3baff
Removed catch to prevent truncating stack trace
michael-wb May 24, 2023
0df6cf0
Updated changelog
michael-wb May 24, 2023
191ddde
Merge branch 'master' of github.com:realm/realm-core into mwb/bad-all…
michael-wb May 25, 2023
22ad033
Added scope exit to ensure AsyncOper object is reclaimed/deleted
michael-wb May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
### Internals
* Simplify the implementation of query expression nodes which have a btree leaf cache.
* Fix a lock order inversion hit by object store tests running on linux. The cycle required test-specific code and so is not applicable to non-tests.
* Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() ([PR #6667](https://github.com/realm/realm-core/pull/6667))

----------------------------------------------

Expand Down
50 changes: 27 additions & 23 deletions src/realm/sync/network/network.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@
#include <realm/status.hpp>
#include <realm/util/features.h>
#include <realm/util/assert.hpp>
#include <realm/util/backtrace.hpp>
#include <realm/util/basic_system_errors.hpp>
#include <realm/util/bind_ptr.hpp>
#include <realm/util/buffer.hpp>
#include <realm/util/misc_ext_errors.hpp>
#include <realm/util/basic_system_errors.hpp>
#include <realm/util/backtrace.hpp>
#include <realm/util/scope_exit.hpp>

// Linux epoll
#if defined(REALM_USE_EPOLL) && !REALM_ANDROID
Expand Down Expand Up @@ -2747,28 +2748,31 @@ inline void Service::AsyncOper::do_recycle_and_execute(bool orphaned, H& handler
// the memory is available for a new post operation that might be initiated
// during the execution of the handler.
bool was_recycled = false;
try {
// We need to copy or move all arguments to be passed to the handler,
// such that there is no risk of references to the recycled operation
// object being passed to the handler (the passed arguments may be
// references to members of the recycled operation object). The easiest
// way to achive this, is by forwarding the reference arguments (passed
// to this function) to a helper function whose arguments have
// nonreference type (`Args...` rather than `Args&&...`).
//
// Note that the copying and moving of arguments may throw, and it is
// important that the operation is still recycled even if that
// happens. For that reason, copying and moving of arguments must not
// happen until we are in a scope (this scope) that catches and deals
// correctly with such exceptions.
do_recycle_and_execute_helper(orphaned, was_recycled, std::move(handler),
std::forward<Args>(args)...); // Throws
}
catch (...) {
if (!was_recycled)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the effect of not doing this anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect any impact. It was only there to ensure the AsyncOper object was either recycled or deleted if an exception occurred during the std::move or std::forward of the provided arguments before the object could be recycled/deleted. I've never seen this happen in practice, so the recycle/delete will have already happened before calling the handler, in addition, if the catch clause is executed, the app is going to crash (or at least be in a bad state) anyways...


// ScopeExit to ensure the AsyncOper object was reclaimed/deleted
auto at_exit = util::ScopeExit([this, &was_recycled, &orphaned]() noexcept {
if (!was_recycled) {
do_recycle(orphaned);
throw;
}
}
});

// We need to copy or move all arguments to be passed to the handler,
// such that there is no risk of references to the recycled operation
// object being passed to the handler (the passed arguments may be
// references to members of the recycled operation object). The easiest
// way to achive this, is by forwarding the reference arguments (passed
// to this function) to a helper function whose arguments have
// nonreference type (`Args...` rather than `Args&&...`).
//
// Note that the copying and moving of arguments may throw, and it is
// important that the operation is still recycled even if that
// happens. For that reason, copying and moving of arguments must not
// happen until we are in a scope (this scope) that catches and deals
// correctly with such exceptions.
do_recycle_and_execute_helper(orphaned, was_recycled, std::move(handler),
std::forward<Args>(args)...); // Throws

// Removed catch to prevent truncating the stack trace on exception
}

template <class H, class... Args>
Expand Down