Skip to content

Commit

Permalink
Allow moving SyncCallback (#43268)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #43268

We previously restricted all copies and moves of SyncCallback, but that led to unsafe calling paths being added instead to AsyncCallback. Instead, allowing moving of SyncCallback, and document the need for the caller to invoke it safely, so can we remove the unsafe path from AsyncCallback.

Changelog: [General][Changed] Allow moving SyncCallback for advanced use-cases

Reviewed By: christophpurrer

Differential Revision: D54381734

fbshipit-source-id: 5fd797cd4541e507aa68f1e4e76a1be5cae20fbe
  • Loading branch information
javache authored and facebook-github-bot committed Mar 4, 2024
1 parent 24e6722 commit 43c55e9
Showing 1 changed file with 16 additions and 12 deletions.
28 changes: 16 additions & 12 deletions packages/react-native/ReactCommon/react/bridging/Function.h
Expand Up @@ -54,15 +54,6 @@ class AsyncCallback {
callWithFunction(priority, std::move(callImpl));
}

/// Invoke the function write-away as if it was a synchronous function
/// without any synchronization or delegating to JS context.
/// @note Caller is responsible for calling this from within JS context.
void unsafeCallSync(Args... args) const noexcept {
if (callback_) {
(*callback_)(std::forward<Args>(args)...);
}
}

private:
friend Bridging<AsyncCallback>;

Expand Down Expand Up @@ -110,6 +101,9 @@ class AsyncCallback {
}
};

// You must ensure that when invoking this you're located on the JS thread, or
// have exclusive control of the JS VM context. If you cannot ensure this, use
// AsyncCallback instead.
template <typename R, typename... Args>
class SyncCallback<R(Args...)> {
public:
Expand All @@ -122,9 +116,19 @@ class SyncCallback<R(Args...)> {
rt,
std::move(jsInvoker))) {}

// Disallow moving to prevent function from get called on another thread.
SyncCallback(SyncCallback&&) = delete;
SyncCallback& operator=(SyncCallback&&) = delete;
// Disallow copying, as we can no longer safely destroy the callback
// from the destructor if there's multiple copies
SyncCallback(const SyncCallback&) = delete;
SyncCallback& operator=(const SyncCallback&) = delete;

// Allow move
SyncCallback(SyncCallback&& other) noexcept
: wrapper_(std::move(other.wrapper_)) {}

SyncCallback& operator=(SyncCallback&& other) noexcept {
wrapper_ = std::move(other.wrapper_);
return *this;
}

~SyncCallback() {
if (auto wrapper = wrapper_.lock()) {
Expand Down

0 comments on commit 43c55e9

Please sign in to comment.