From 8a341b1e8710f43b861e6ce1d29a78de20f3e91b Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Fri, 20 Dec 2024 18:47:01 +0100 Subject: [PATCH 1/7] fix: Reject Promise with timeout if it gets destroyed If a Promise gets destroyed without ever being resolved or rejected, it goes stale. This PR fixes this and rejects a Promise with a timeout error if it gets destructed and is still pending --- .../cpp/core/Promise.hpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/react-native-nitro-modules/cpp/core/Promise.hpp b/packages/react-native-nitro-modules/cpp/core/Promise.hpp index 51c39596d..7d594f2af 100644 --- a/packages/react-native-nitro-modules/cpp/core/Promise.hpp +++ b/packages/react-native-nitro-modules/cpp/core/Promise.hpp @@ -32,6 +32,14 @@ class Promise final { private: Promise() {} +public: + ~Promise() { + if (isPending()) [[unlikely]] { + std::runtime_error error("Timeouted: Promise was destroyed!"); + reject(std::make_exception_ptr(std::move(error))); + } + } + public: /** * Creates a new pending Promise that has to be resolved @@ -258,6 +266,14 @@ class Promise final { private: Promise() {} +public: + ~Promise() { + if (isPending()) [[unlikely]] { + std::runtime_error error("Timeouted: Promise was destroyed!"); + reject(std::make_exception_ptr(std::move(error))); + } + } + public: static std::shared_ptr create() { return std::shared_ptr(new Promise()); From 90d7410fc31670b91fdd14d7f7690360fcf58bfc Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Fri, 20 Dec 2024 19:50:47 +0100 Subject: [PATCH 2/7] Add promise typename --- packages/react-native-nitro-modules/cpp/core/Promise.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-native-nitro-modules/cpp/core/Promise.hpp b/packages/react-native-nitro-modules/cpp/core/Promise.hpp index 7d594f2af..207b3b737 100644 --- a/packages/react-native-nitro-modules/cpp/core/Promise.hpp +++ b/packages/react-native-nitro-modules/cpp/core/Promise.hpp @@ -35,8 +35,8 @@ class Promise final { public: ~Promise() { if (isPending()) [[unlikely]] { - std::runtime_error error("Timeouted: Promise was destroyed!"); - reject(std::make_exception_ptr(std::move(error))); + auto message = std::string("Timeouted: Promise<") + TypeInfo::getFriendlyTypeName() + "> was destroyed!"; + reject(std::make_exception_ptr(std::runtime_error(message))); } } @@ -269,7 +269,7 @@ class Promise final { public: ~Promise() { if (isPending()) [[unlikely]] { - std::runtime_error error("Timeouted: Promise was destroyed!"); + std::runtime_error error("Timeouted: Promise was destroyed!"); reject(std::make_exception_ptr(std::move(error))); } } From 1fb4ade740b559cd1b46ff421727ace36989b9a5 Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Fri, 20 Dec 2024 19:53:27 +0100 Subject: [PATCH 3/7] Reject swift promise as well --- packages/react-native-nitro-modules/ios/core/Promise.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-native-nitro-modules/ios/core/Promise.swift b/packages/react-native-nitro-modules/ios/core/Promise.swift index 8572eee1c..4ca5412b4 100644 --- a/packages/react-native-nitro-modules/ios/core/Promise.swift +++ b/packages/react-native-nitro-modules/ios/core/Promise.swift @@ -37,7 +37,8 @@ public final class Promise { deinit { if state == nil { - print("⚠️ Promise<\(String(describing: T.self))> got destroyed, but was never resolved or rejected! It is probably left hanging in JS now.") + let message = "Timeouted: Promise<\(String(describing: T.self))> was destroyed!" + reject(RuntimeError.error(withMessage: message)) } } From 47d48c06af7785d9efffee8f1e50cd6d2d59cf72 Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Sun, 22 Dec 2024 13:31:32 +0100 Subject: [PATCH 4/7] Update Promise.hpp --- packages/react-native-nitro-modules/cpp/core/Promise.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native-nitro-modules/cpp/core/Promise.hpp b/packages/react-native-nitro-modules/cpp/core/Promise.hpp index 207b3b737..8ae10b6e0 100644 --- a/packages/react-native-nitro-modules/cpp/core/Promise.hpp +++ b/packages/react-native-nitro-modules/cpp/core/Promise.hpp @@ -35,7 +35,7 @@ class Promise final { public: ~Promise() { if (isPending()) [[unlikely]] { - auto message = std::string("Timeouted: Promise<") + TypeInfo::getFriendlyTypeName() + "> was destroyed!"; + auto message = std::string("Timeouted: Promise<") + TypeInfo::getFriendlyTypename() + "> was destroyed!"; reject(std::make_exception_ptr(std::runtime_error(message))); } } From c5d48851e2ac72a306788e5706530a057fcaf975 Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Sun, 22 Dec 2024 13:35:04 +0100 Subject: [PATCH 5/7] fix: Build --- .../react-native-nitro-image/ios/HybridTestObjectSwift.swift | 2 +- packages/react-native-nitro-modules/cpp/core/Promise.hpp | 2 +- packages/react-native-nitro-modules/ios/core/Promise.swift | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-native-nitro-image/ios/HybridTestObjectSwift.swift b/packages/react-native-nitro-image/ios/HybridTestObjectSwift.swift index d49f51f4c..b873fc37b 100644 --- a/packages/react-native-nitro-image/ios/HybridTestObjectSwift.swift +++ b/packages/react-native-nitro-image/ios/HybridTestObjectSwift.swift @@ -74,7 +74,7 @@ class HybridTestObjectSwift : HybridTestObjectSwiftKotlinSpec { func callSumUpNTimes(callback: @escaping (() -> Promise), n: Double) throws -> Promise { var result = 0.0 return Promise.async { - for i in 1...Int(n) { + for _ in 1...Int(n) { let current = try await callback().await() result += current } diff --git a/packages/react-native-nitro-modules/cpp/core/Promise.hpp b/packages/react-native-nitro-modules/cpp/core/Promise.hpp index 8ae10b6e0..48aba72f5 100644 --- a/packages/react-native-nitro-modules/cpp/core/Promise.hpp +++ b/packages/react-native-nitro-modules/cpp/core/Promise.hpp @@ -35,7 +35,7 @@ class Promise final { public: ~Promise() { if (isPending()) [[unlikely]] { - auto message = std::string("Timeouted: Promise<") + TypeInfo::getFriendlyTypename() + "> was destroyed!"; + auto message = std::string("Timeouted: Promise<") + TypeInfo::getFriendlyTypename() + "> was destroyed!"; reject(std::make_exception_ptr(std::runtime_error(message))); } } diff --git a/packages/react-native-nitro-modules/ios/core/Promise.swift b/packages/react-native-nitro-modules/ios/core/Promise.swift index 4ca5412b4..79cb1dccd 100644 --- a/packages/react-native-nitro-modules/ios/core/Promise.swift +++ b/packages/react-native-nitro-modules/ios/core/Promise.swift @@ -38,7 +38,7 @@ public final class Promise { deinit { if state == nil { let message = "Timeouted: Promise<\(String(describing: T.self))> was destroyed!" - reject(RuntimeError.error(withMessage: message)) + reject(withError: RuntimeError.error(withMessage: message)) } } From c3bfd3de5a07a5c86e2ef9c5106007b36eef0ca3 Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Sun, 22 Dec 2024 13:42:18 +0100 Subject: [PATCH 6/7] Update Dispatcher.cpp --- .../cpp/threading/Dispatcher.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/react-native-nitro-modules/cpp/threading/Dispatcher.cpp b/packages/react-native-nitro-modules/cpp/threading/Dispatcher.cpp index f3a83225b..7c41097a1 100644 --- a/packages/react-native-nitro-modules/cpp/threading/Dispatcher.cpp +++ b/packages/react-native-nitro-modules/cpp/threading/Dispatcher.cpp @@ -1,3 +1,10 @@ +// +// Dispatcher.cpp +// react-native-nitro +// +// Created by Marc Rousavy on 22.07.24. +// + #include "Dispatcher.hpp" #include "JSIHelpers.hpp" #include "NitroDefines.hpp" From 28666d9aeab1cfca2f66c1c0f09468ba1976652c Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Sun, 22 Dec 2024 13:50:55 +0100 Subject: [PATCH 7/7] fix: Also reject `JPromise` if it gets destroyed --- .../android/src/main/cpp/core/JPromise.hpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-native-nitro-modules/android/src/main/cpp/core/JPromise.hpp b/packages/react-native-nitro-modules/android/src/main/cpp/core/JPromise.hpp index 28c032eb6..803d516c3 100644 --- a/packages/react-native-nitro-modules/android/src/main/cpp/core/JPromise.hpp +++ b/packages/react-native-nitro-modules/android/src/main/cpp/core/JPromise.hpp @@ -56,6 +56,16 @@ class JPromise final : public jni::HybridClass { return newObjectCxxArgs(); } +public: + ~JPromise() override { + if (_result == nullptr && _error == nullptr) [[unlikely]] { + jni::ThreadScope::WithClassLoader([&]() { + std::runtime_error error("Timeouted: JPromise was destroyed!"); + this->reject(jni::getJavaExceptionForCppException(std::make_exception_ptr(error))); + }); + } + } + public: void resolve(jni::alias_ref result) { std::unique_lock lock(_mutex);