From 0c5c75e8a4b73458ebbcb15f2d9e373ed1673855 Mon Sep 17 00:00:00 2001 From: Ali Mohammad Pur Date: Sat, 8 Jul 2023 01:30:27 +0330 Subject: [PATCH] LibCore: Slightly rework the Core::Promise API The previous iteration of this API was somewhat odd and rough in random places, which degraded usability and made less than perfect sense. This commit reworks the API to be a little closer to more conventional promise APIs (a la javascript promises). Also adds a test to ensure the class even works. --- Tests/LibCore/CMakeLists.txt | 5 +- Tests/LibCore/TestLibCorePromise.cpp | 59 ++++++++++++ Userland/Applications/Browser/CookieJar.cpp | 4 +- Userland/Libraries/LibCore/Forward.h | 4 +- Userland/Libraries/LibCore/Promise.h | 91 +++++++++++++++---- .../Libraries/LibCore/ThreadEventQueue.cpp | 4 +- Userland/Libraries/LibIMAP/Client.cpp | 8 +- .../Libraries/LibThreading/BackgroundAction.h | 6 +- Userland/Services/WebDriver/Session.cpp | 7 +- 9 files changed, 150 insertions(+), 38 deletions(-) create mode 100644 Tests/LibCore/TestLibCorePromise.cpp diff --git a/Tests/LibCore/CMakeLists.txt b/Tests/LibCore/CMakeLists.txt index a4a568bb90..afbc6a364e 100644 --- a/Tests/LibCore/CMakeLists.txt +++ b/Tests/LibCore/CMakeLists.txt @@ -1,10 +1,11 @@ set(TEST_SOURCES TestLibCoreArgsParser.cpp - TestLibCoreFileWatcher.cpp TestLibCoreDeferredInvoke.cpp - TestLibCoreStream.cpp TestLibCoreFilePermissionsMask.cpp + TestLibCoreFileWatcher.cpp + TestLibCorePromise.cpp TestLibCoreSharedSingleProducerCircularQueue.cpp + TestLibCoreStream.cpp ) foreach(source IN LISTS TEST_SOURCES) diff --git a/Tests/LibCore/TestLibCorePromise.cpp b/Tests/LibCore/TestLibCorePromise.cpp new file mode 100644 index 0000000000..c8348051f2 --- /dev/null +++ b/Tests/LibCore/TestLibCorePromise.cpp @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2023, Ali Mohammad Pur + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include + +TEST_CASE(promise_await_async_event) +{ + Core::EventLoop loop; + + auto promise = MUST(Core::Promise::try_create()); + + loop.deferred_invoke([=] { + promise->resolve(42); + }); + + auto& result = promise->await(); + EXPECT(!result.is_error()); + EXPECT_EQ(result.value(), 42); +} + +TEST_CASE(promise_await_async_event_rejection) +{ + Core::EventLoop loop; + + auto promise = MUST(Core::Promise::try_create()); + + loop.deferred_invoke([=] { + promise->reject(AK::Error::from_string_literal("lol no")); + }); + + auto& result = promise->await(); + EXPECT(result.is_error()); + EXPECT_EQ(result.error().string_literal(), "lol no"sv); +} + +TEST_CASE(promise_chain_handlers) +{ + Core::EventLoop loop; + + bool resolved = false; + bool rejected = false; + + NonnullRefPtr> promise = MUST(Core::Promise::try_create()) + ->when_resolved([&](int&) -> ErrorOr { resolved = true; return {}; }) + .when_rejected([&](AK::Error const&) { rejected = true; }); + + loop.deferred_invoke([=] { + promise->resolve(42); + }); + + promise->await(); + EXPECT(resolved); + EXPECT(!rejected); +} diff --git a/Userland/Applications/Browser/CookieJar.cpp b/Userland/Applications/Browser/CookieJar.cpp index 0f2abb6c0b..6e5dc6b444 100644 --- a/Userland/Applications/Browser/CookieJar.cpp +++ b/Userland/Applications/Browser/CookieJar.cpp @@ -608,10 +608,10 @@ void CookieJar::select_all_cookies_from_database(OnSelectAllCookiesResult on_res on_result(cookie.release_value()); }, [&]() { - MUST(promise->resolve({})); + promise->resolve({}); }, [&](auto) { - MUST(promise->resolve({})); + promise->resolve({}); }); MUST(promise->await()); diff --git a/Userland/Libraries/LibCore/Forward.h b/Userland/Libraries/LibCore/Forward.h index 2e1092f7f1..9272a542d7 100644 --- a/Userland/Libraries/LibCore/Forward.h +++ b/Userland/Libraries/LibCore/Forward.h @@ -6,6 +6,8 @@ #pragma once +#include + namespace Core { class AnonymousBuffer; @@ -32,7 +34,7 @@ class Object; class ObjectClassRegistration; class ProcessStatisticsReader; class Socket; -template +template class Promise; class SocketAddress; class TCPServer; diff --git a/Userland/Libraries/LibCore/Promise.h b/Userland/Libraries/LibCore/Promise.h index a996eb7d45..4bce2895e2 100644 --- a/Userland/Libraries/LibCore/Promise.h +++ b/Userland/Libraries/LibCore/Promise.h @@ -1,76 +1,127 @@ /* * Copyright (c) 2021, Kyle Pereira * Copyright (c) 2022, kleines Filmröllchen + * Copyright (c) 2021-2023, Ali Mohammad Pur * * SPDX-License-Identifier: BSD-2-Clause */ #pragma once +#include #include #include namespace Core { -template +template class Promise : public Object { C_OBJECT(Promise); public: - Function(Result&)> on_resolved; + using ErrorType = TError; - ErrorOr resolve(Result&& result) + Function(Result&)> on_resolution; + Function on_rejection; + + void resolve(Result&& result) { - m_pending_or_error = move(result); + m_result_or_rejection = move(result); - if (on_resolved) - return on_resolved(m_pending_or_error->value()); - return {}; + if (on_resolution) { + auto handler_result = on_resolution(m_result_or_rejection->value()); + possibly_handle_rejection(handler_result); + } } - void cancel(Error error) + void reject(ErrorType&& error) { - m_pending_or_error = move(error); + m_result_or_rejection = move(error); + possibly_handle_rejection(*m_result_or_rejection); } - bool is_canceled() + bool is_rejected() { - return m_pending_or_error.has_value() && m_pending_or_error->is_error(); + return m_result_or_rejection.has_value() && m_result_or_rejection->is_error(); } bool is_resolved() const { - return m_pending_or_error.has_value() && !m_pending_or_error->is_error(); + return m_result_or_rejection.has_value() && !m_result_or_rejection->is_error(); } - ErrorOr await() + ErrorOr& await() { - while (!m_pending_or_error.has_value()) + while (!m_result_or_rejection.has_value()) Core::EventLoop::current().pump(); - return m_pending_or_error.release_value(); + return *m_result_or_rejection; } // Converts a Promise to a Promise using a function func: A -> B template - RefPtr> map(T func(Result&)) + RefPtr> map(Function func) { RefPtr> new_promise = Promise::construct(); - on_resolved = [new_promise, func](Result& result) { - auto t = func(result); - return new_promise->resolve(move(t)); + on_resolution = [new_promise, func = move(func)](Result& result) -> ErrorOr { + new_promise->resolve(func(result)); + return {}; + }; + on_rejection = [new_promise](ErrorType& error) { + new_promise->reject(move(error)); }; return new_promise; } + template F> + Promise& when_resolved(F handler) + { + on_resolution = [handler = move(handler)](Result& result) { return handler(result); }; + if (is_resolved()) { + auto handler_result = on_resolution(m_result_or_rejection->value()); + possibly_handle_rejection(handler_result); + } + + return *this; + } + + template, Result&> F> + Promise& when_resolved(F handler) + { + on_resolution = move(handler); + if (is_resolved()) { + auto handler_result = on_resolution(m_result_or_rejection->value()); + possibly_handle_rejection(handler_result); + } + + return *this; + } + + template F> + Promise& when_rejected(F handler) + { + on_rejection = move(handler); + if (is_rejected()) + on_rejection(m_result_or_rejection->error()); + + return *this; + } + private: + template + void possibly_handle_rejection(ErrorOr& result) + { + if (result.is_error() && on_rejection) + on_rejection(result.error()); + } + Promise() = default; Promise(Object* parent) : Object(parent) { } - Optional> m_pending_or_error; + Optional> m_result_or_rejection; }; } diff --git a/Userland/Libraries/LibCore/ThreadEventQueue.cpp b/Userland/Libraries/LibCore/ThreadEventQueue.cpp index 6bd5514d4f..0aec61c151 100644 --- a/Userland/Libraries/LibCore/ThreadEventQueue.cpp +++ b/Userland/Libraries/LibCore/ThreadEventQueue.cpp @@ -76,7 +76,7 @@ void ThreadEventQueue::cancel_all_pending_jobs() { Threading::MutexLocker lock(m_private->mutex); for (auto const& promise : m_private->pending_promises) - promise->cancel(Error::from_errno(ECANCELED)); + promise->reject(Error::from_errno(ECANCELED)); m_private->pending_promises.clear(); } @@ -87,7 +87,7 @@ size_t ThreadEventQueue::process() { Threading::MutexLocker locker(m_private->mutex); events = move(m_private->queued_events); - m_private->pending_promises.remove_all_matching([](auto& job) { return job->is_resolved() || job->is_canceled(); }); + m_private->pending_promises.remove_all_matching([](auto& job) { return job->is_resolved() || job->is_rejected(); }); } size_t processed_events = 0; diff --git a/Userland/Libraries/LibIMAP/Client.cpp b/Userland/Libraries/LibIMAP/Client.cpp index 753af26ad9..5bf83f70ad 100644 --- a/Userland/Libraries/LibIMAP/Client.cpp +++ b/Userland/Libraries/LibIMAP/Client.cpp @@ -64,7 +64,7 @@ ErrorOr Client::on_ready_to_receive() // Once we get server hello we can start sending. if (m_connect_pending) { - TRY(m_connect_pending->resolve({})); + m_connect_pending->resolve({}); m_connect_pending.clear(); m_buffer.clear(); return {}; @@ -227,13 +227,13 @@ ErrorOr Client::handle_parsed_response(ParseStatus&& parse_status) bool should_send_next = false; if (!parse_status.successful) { m_expecting_response = false; - TRY(m_pending_promises.first()->resolve({})); + m_pending_promises.first()->resolve({}); m_pending_promises.remove(0); } if (parse_status.response.has_value()) { m_expecting_response = false; should_send_next = parse_status.response->has(); - TRY(m_pending_promises.first()->resolve(move(parse_status.response))); + m_pending_promises.first()->resolve(move(parse_status.response)); m_pending_promises.remove(0); } @@ -386,7 +386,7 @@ RefPtr>> Client::append(StringView mailbox, Mess auto response_promise = Promise>::construct(); m_pending_promises.append(response_promise); - continue_req->on_resolved = [this, message2 { move(message) }](auto& data) -> ErrorOr { + continue_req->on_resolution = [this, message2 { move(message) }](auto& data) -> ErrorOr { if (!data.has_value()) { TRY(handle_parsed_response({ .successful = false, .response = {} })); } else { diff --git a/Userland/Libraries/LibThreading/BackgroundAction.h b/Userland/Libraries/LibThreading/BackgroundAction.h index 9376e7e996..0b513a3075 100644 --- a/Userland/Libraries/LibThreading/BackgroundAction.h +++ b/Userland/Libraries/LibThreading/BackgroundAction.h @@ -61,7 +61,7 @@ private: , m_on_complete(move(on_complete)) { if (m_on_complete) { - m_promise->on_resolved = [](NonnullRefPtr& object) -> ErrorOr { + m_promise->on_resolution = [](NonnullRefPtr& object) -> ErrorOr { auto self = static_ptr_cast>(object); VERIFY(self->m_result.has_value()); if (auto maybe_error = self->m_on_complete(self->m_result.value()); maybe_error.is_error()) @@ -78,7 +78,7 @@ private: enqueue_work([this, origin_event_loop = &Core::EventLoop::current()] { auto result = m_action(*this); // The event loop cancels the promise when it exits. - m_canceled |= m_promise->is_canceled(); + m_canceled |= m_promise->is_rejected(); auto callback_scheduled = false; // All of our work was successful and we weren't cancelled; resolve the event loop's promise. if (!m_canceled && !result.is_error()) { @@ -99,7 +99,7 @@ private: if (result.is_error()) error = result.release_error(); - m_promise->cancel(Error::from_errno(ECANCELED)); + m_promise->reject(Error::from_errno(ECANCELED)); if (!m_canceled && m_on_error) { callback_scheduled = true; origin_event_loop->deferred_invoke([this, error = move(error)]() mutable { diff --git a/Userland/Services/WebDriver/Session.cpp b/Userland/Services/WebDriver/Session.cpp index 3a029fcbbe..9a8aab1fee 100644 --- a/Userland/Services/WebDriver/Session.cpp +++ b/Userland/Services/WebDriver/Session.cpp @@ -65,8 +65,7 @@ ErrorOr> Session::create_server(NonnullRefPtron_accept = [this, promise](auto client_socket) { auto maybe_connection = adopt_nonnull_ref_or_enomem(new (nothrow) WebContentConnection(move(client_socket))); if (maybe_connection.is_error()) { - // Use of MUST in this function is safe, as our promise callback can never error out. - MUST(promise->resolve(maybe_connection.release_error())); + promise->resolve(maybe_connection.release_error()); return; } @@ -85,11 +84,11 @@ ErrorOr> Session::create_server(NonnullRefPtrresolve({})); + promise->resolve({}); }; server->on_accept_error = [promise](auto error) { - MUST(promise->resolve(move(error))); + promise->resolve(move(error)); }; return server;