From 4183443f02d3762d6c0a84f3c7473fd4d8407a6f Mon Sep 17 00:00:00 2001 From: grorp Date: Tue, 12 Mar 2024 20:09:43 +0100 Subject: [PATCH] Allow sync HTTP fetches to be interrupted to fix hanging (#14412) Co-authored-by: Jude Melton-Houghton --- src/gui/guiEngine.cpp | 6 ++++-- src/httpfetch.cpp | 32 ++++++++++++++++++++++++++++---- src/httpfetch.h | 9 ++++++--- src/script/lua_api/l_http.cpp | 4 ++-- src/threading/thread.cpp | 11 +++++++++++ src/threading/thread.h | 5 +++++ 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/gui/guiEngine.cpp b/src/gui/guiEngine.cpp index fd1121346..dfc7f70ed 100644 --- a/src/gui/guiEngine.cpp +++ b/src/gui/guiEngine.cpp @@ -608,13 +608,15 @@ bool GUIEngine::downloadFile(const std::string &url, const std::string &target) fetch_request.caller = HTTPFETCH_SYNC; fetch_request.timeout = std::max(MIN_HTTPFETCH_TIMEOUT, (long)g_settings->getS32("curl_file_download_timeout")); - httpfetch_sync(fetch_request, fetch_result); + bool completed = httpfetch_sync_interruptible(fetch_request, fetch_result); - if (!fetch_result.succeeded) { + if (!completed || !fetch_result.succeeded) { target_file.close(); fs::DeleteSingleFileOrEmptyDirectory(target); return false; } + // TODO: directly stream the response data into the file instead of first + // storing the complete response in memory target_file << fetch_result.data; return true; diff --git a/src/httpfetch.cpp b/src/httpfetch.cpp index a966d4855..652313979 100644 --- a/src/httpfetch.cpp +++ b/src/httpfetch.cpp @@ -31,6 +31,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "exceptions.h" #include "debug.h" #include "log.h" +#include "porting.h" #include "util/container.h" #include "util/thread.h" #include "version.h" @@ -783,7 +784,7 @@ static void httpfetch_request_clear(u64 caller) } } -void httpfetch_sync(const HTTPFetchRequest &fetch_request, +static void httpfetch_sync(const HTTPFetchRequest &fetch_request, HTTPFetchResult &fetch_result) { // Create ongoing fetch data and make a cURL handle @@ -796,6 +797,28 @@ void httpfetch_sync(const HTTPFetchRequest &fetch_request, fetch_result = *ongoing.complete(res); } +bool httpfetch_sync_interruptible(const HTTPFetchRequest &fetch_request, + HTTPFetchResult &fetch_result, long interval) +{ + if (Thread *thread = Thread::getCurrentThread()) { + HTTPFetchRequest req = fetch_request; + req.caller = httpfetch_caller_alloc_secure(); + httpfetch_async(req); + do { + if (thread->stopRequested()) { + httpfetch_caller_free(req.caller); + fetch_result = HTTPFetchResult(fetch_request); + return false; + } + sleep_ms(interval); + } while (!httpfetch_async_get(req.caller, fetch_result)); + httpfetch_caller_free(req.caller); + } else { + httpfetch_sync(fetch_request, fetch_result); + } + return true; +} + #else // USE_CURL /* @@ -825,13 +848,14 @@ static void httpfetch_request_clear(u64 caller) { } -void httpfetch_sync(const HTTPFetchRequest &fetch_request, - HTTPFetchResult &fetch_result) +bool httpfetch_sync_interruptible(const HTTPFetchRequest &fetch_request, + HTTPFetchResult &fetch_result, long interval) { - errorstream << "httpfetch_sync: unable to fetch " << fetch_request.url + errorstream << "httpfetch_sync_interruptible: unable to fetch " << fetch_request.url << " because USE_CURL=0" << std::endl; fetch_result = HTTPFetchResult(fetch_request); // sets succeeded = false etc. + return false; } #endif // USE_CURL diff --git a/src/httpfetch.h b/src/httpfetch.h index 930ce591c..88a4cd727 100644 --- a/src/httpfetch.h +++ b/src/httpfetch.h @@ -135,6 +135,9 @@ u64 httpfetch_caller_alloc_secure(); // to stop any ongoing fetches for the given caller. void httpfetch_caller_free(u64 caller); -// Performs a synchronous HTTP request. This blocks and therefore should -// only be used from background threads. -void httpfetch_sync(const HTTPFetchRequest &fetch_request, HTTPFetchResult &fetch_result); +// Performs a synchronous HTTP request that is interruptible if the current +// thread is a Thread object. interval is the completion check interval in ms. +// This blocks and therefore should only be used from background threads. +// Returned is whether the request completed without interruption. +bool httpfetch_sync_interruptible(const HTTPFetchRequest &fetch_request, + HTTPFetchResult &fetch_result, long interval = 100); diff --git a/src/script/lua_api/l_http.cpp b/src/script/lua_api/l_http.cpp index 5566a8523..57f632291 100644 --- a/src/script/lua_api/l_http.cpp +++ b/src/script/lua_api/l_http.cpp @@ -114,9 +114,9 @@ int ModApiHttp::l_http_fetch_sync(lua_State *L) infostream << "Mod performs HTTP request with URL " << req.url << std::endl; HTTPFetchResult res; - httpfetch_sync(req, res); + bool completed = httpfetch_sync_interruptible(req, res); - push_http_fetch_result(L, res, true); + push_http_fetch_result(L, res, completed); return 1; } diff --git a/src/threading/thread.cpp b/src/threading/thread.cpp index ef639ffef..6baa301f6 100644 --- a/src/threading/thread.cpp +++ b/src/threading/thread.cpp @@ -60,6 +60,9 @@ DEALINGS IN THE SOFTWARE. #endif +thread_local Thread *current_thread = nullptr; + + Thread::Thread(const std::string &name) : m_name(name), m_request_stop(false), @@ -176,6 +179,8 @@ void Thread::threadProc(Thread *thr) thr->m_kernel_thread_id = thread_self(); #endif + current_thread = thr; + thr->setName(thr->m_name); g_logger.registerThread(thr->m_name); @@ -196,6 +201,12 @@ void Thread::threadProc(Thread *thr) } +Thread *Thread::getCurrentThread() +{ + return current_thread; +} + + void Thread::setName(const std::string &name) { #if defined(__linux__) diff --git a/src/threading/thread.h b/src/threading/thread.h index 45fb171da..2d0641b43 100644 --- a/src/threading/thread.h +++ b/src/threading/thread.h @@ -119,6 +119,11 @@ public: */ bool setPriority(int prio); + /* + * Returns the thread object of the current thread if it exists. + */ + static Thread *getCurrentThread(); + /* * Sets the currently executing thread's name to where supported; useful * for debugging.