From 2693745336ddc6c7a790ecf8a760aa74651ac055 Mon Sep 17 00:00:00 2001 From: Lucas CHOLLET Date: Mon, 12 Dec 2022 23:34:28 +0100 Subject: [PATCH] LibThreading+Everywhere: Support returning error from `BackgroundAction` This patch allows returning an `Error` from the `on_complete` callback in `BackgroundAction`. It also adds a custom callback to manage errors returned during its execution. --- Userland/Applications/Assistant/Providers.cpp | 6 +++-- Userland/Applications/CrashReporter/main.cpp | 23 +++++++++++-------- .../DisplaySettings/MonitorWidget.cpp | 6 +++-- .../SystemMonitor/ThreadStackWidget.cpp | 5 ++-- Userland/Libraries/LibGUI/FileSystemModel.cpp | 5 ++-- .../Libraries/LibThreading/BackgroundAction.h | 14 ++++++++--- 6 files changed, 38 insertions(+), 21 deletions(-) diff --git a/Userland/Applications/Assistant/Providers.cpp b/Userland/Applications/Assistant/Providers.cpp index 9d82f6e8fdb..0e679beae73 100644 --- a/Userland/Applications/Assistant/Providers.cpp +++ b/Userland/Applications/Assistant/Providers.cpp @@ -144,8 +144,9 @@ void FileProvider::query(DeprecatedString const& query, Function ErrorOr { on_complete(move(results)); + return {}; }); } @@ -192,8 +193,9 @@ void FileProvider::build_filesystem_cache() dbgln("Built cache in {} ms", timer.elapsed()); return 0; }, - [this](auto) { + [this](auto) -> ErrorOr { m_building_cache = false; + return {}; }); } diff --git a/Userland/Applications/CrashReporter/main.cpp b/Userland/Applications/CrashReporter/main.cpp index 597f92123be..ae21f397ce0 100644 --- a/Userland/Applications/CrashReporter/main.cpp +++ b/Userland/Applications/CrashReporter/main.cpp @@ -308,26 +308,24 @@ ErrorOr serenity_main(Main::Arguments arguments) }); return results; }, - [&](auto results) { - // FIXME: Make BackgroundAction propagate ErrorOr values so we can replace these MUSTs with TRYs. - + [&](auto results) -> ErrorOr { for (auto& backtrace : results.thread_backtraces) { - auto container = MUST(backtrace_tab_widget->try_add_tab(backtrace.title)); - (void)MUST(container->template try_set_layout()); + auto container = TRY(backtrace_tab_widget->try_add_tab(backtrace.title)); + (void)TRY(container->template try_set_layout()); container->layout()->set_margins(4); - auto backtrace_text_editor = MUST(container->template try_add()); + auto backtrace_text_editor = TRY(container->template try_add()); backtrace_text_editor->set_text(backtrace.text); backtrace_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); backtrace_text_editor->set_wrapping_mode(GUI::TextEditor::WrappingMode::NoWrap); backtrace_text_editor->set_should_hide_unnecessary_scrollbars(true); - full_backtrace.appendff("==== {} ====\n{}\n", backtrace.title, backtrace.text); + TRY(full_backtrace.try_appendff("==== {} ====\n{}\n", backtrace.title, backtrace.text)); } for (auto& cpu_registers : results.thread_cpu_registers) { - auto container = MUST(cpu_registers_tab_widget->try_add_tab(cpu_registers.title)); - (void)MUST(container->template try_set_layout()); + auto container = TRY(cpu_registers_tab_widget->try_add_tab(cpu_registers.title)); + (void)TRY(container->template try_set_layout()); container->layout()->set_margins(4); - auto cpu_registers_text_editor = MUST(container->template try_add()); + auto cpu_registers_text_editor = TRY(container->template try_add()); cpu_registers_text_editor->set_text(cpu_registers.text); cpu_registers_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); cpu_registers_text_editor->set_wrapping_mode(GUI::TextEditor::WrappingMode::NoWrap); @@ -339,6 +337,11 @@ ErrorOr serenity_main(Main::Arguments arguments) save_backtrace_button.set_enabled(true); window->resize(window->width(), max(340, window->height())); window->set_progress(0); + return {}; + }, + [window](Error error) { + dbgln("Error while parsing the coredump: {}", error); + window->close(); }); window->show(); diff --git a/Userland/Applications/DisplaySettings/MonitorWidget.cpp b/Userland/Applications/DisplaySettings/MonitorWidget.cpp index 837b8fdcdb7..6a25b532371 100644 --- a/Userland/Applications/DisplaySettings/MonitorWidget.cpp +++ b/Userland/Applications/DisplaySettings/MonitorWidget.cpp @@ -37,17 +37,19 @@ bool MonitorWidget::set_wallpaper(DeprecatedString path) return Gfx::Bitmap::try_load_from_file(path); }, - [this, path](ErrorOr> bitmap_or_error) { + [this, path](ErrorOr> bitmap_or_error) -> ErrorOr { // If we've been requested to change while we were loading the bitmap, don't bother spending the cost to // move and render the now stale bitmap. if (is_different_to_current_wallpaper_path(path)) - return; + return {}; if (bitmap_or_error.is_error()) m_wallpaper_bitmap = nullptr; else m_wallpaper_bitmap = bitmap_or_error.release_value(); m_desktop_dirty = true; update(); + + return bitmap_or_error.is_error() ? bitmap_or_error.release_error() : ErrorOr {}; }); if (path.is_empty()) diff --git a/Userland/Applications/SystemMonitor/ThreadStackWidget.cpp b/Userland/Applications/SystemMonitor/ThreadStackWidget.cpp index eedd49db96f..c5602993651 100644 --- a/Userland/Applications/SystemMonitor/ThreadStackWidget.cpp +++ b/Userland/Applications/SystemMonitor/ThreadStackWidget.cpp @@ -120,10 +120,11 @@ void ThreadStackWidget::refresh() return Symbolication::symbolicate_thread(pid, tid, Symbolication::IncludeSourcePosition::No); }, - [weak_this = make_weak_ptr()](auto result) { + [weak_this = make_weak_ptr()](auto result) -> ErrorOr { if (!weak_this) - return; + return {}; Core::EventLoop::current().post_event(const_cast(*weak_this), make(move(result))); + return {}; }); } diff --git a/Userland/Libraries/LibGUI/FileSystemModel.cpp b/Userland/Libraries/LibGUI/FileSystemModel.cpp index cf37edac5bd..f6d7919da1d 100644 --- a/Userland/Libraries/LibGUI/FileSystemModel.cpp +++ b/Userland/Libraries/LibGUI/FileSystemModel.cpp @@ -670,7 +670,7 @@ bool FileSystemModel::fetch_thumbnail_for(Node const& node) return render_thumbnail(path); }, - [this, path, weak_this](auto thumbnail_or_error) { + [this, path, weak_this](auto thumbnail_or_error) -> ErrorOr { if (thumbnail_or_error.is_error()) { s_thumbnail_cache.set(path, nullptr); dbgln("Failed to load thumbnail for {}: {}", path, thumbnail_or_error.error()); @@ -681,7 +681,7 @@ bool FileSystemModel::fetch_thumbnail_for(Node const& node) // The model was destroyed, no need to update // progress or call any event handlers. if (weak_this.is_null()) - return; + return {}; m_thumbnail_progress++; if (on_thumbnail_progress) @@ -692,6 +692,7 @@ bool FileSystemModel::fetch_thumbnail_for(Node const& node) } did_update(UpdateFlag::DontInvalidateIndices); + return {}; }); return false; diff --git a/Userland/Libraries/LibThreading/BackgroundAction.h b/Userland/Libraries/LibThreading/BackgroundAction.h index a5678511766..54c839caa35 100644 --- a/Userland/Libraries/LibThreading/BackgroundAction.h +++ b/Userland/Libraries/LibThreading/BackgroundAction.h @@ -52,16 +52,21 @@ public: virtual ~BackgroundAction() = default; private: - BackgroundAction(Function action, Function on_complete) + BackgroundAction(Function action, Function(Result)> on_complete, Optional> on_error = {}) : Core::Object(&background_thread()) , m_action(move(action)) , m_on_complete(move(on_complete)) { + if (on_error.has_value()) + m_on_error = on_error.release_value(); + enqueue_work([this, origin_event_loop = &Core::EventLoop::current()] { m_result = m_action(*this); if (m_on_complete) { origin_event_loop->deferred_invoke([this] { - m_on_complete(m_result.release_value()); + auto maybe_error = m_on_complete(m_result.release_value()); + if (maybe_error.is_error()) + m_on_error(maybe_error.release_error()); remove_from_parent(); }); origin_event_loop->wake(); @@ -73,7 +78,10 @@ private: bool m_cancelled { false }; Function m_action; - Function m_on_complete; + Function(Result)> m_on_complete; + Function m_on_error = [](Error error) { + dbgln("Error occurred while running a BackgroundAction: {}", error); + }; Optional m_result; };