From 7e66e77d89e62d11cee88af88449619a1ebcb313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Lucas=20Golini?= Date: Tue, 14 Jul 2026 02:16:01 -0300 Subject: [PATCH] fix: harden network and font resource lifetimes - retain asynchronous HTTP operations until execution or queue disposal - make HTTP shutdown cancel and join queued and running shared-pool requests - avoid deadlocks when callbacks clear the HTTP pool - share atomic cancellation state across request copies - detect socket creation failures before configuring TCP or UDP sockets - release partially initialized mbedTLS state after failed handshakes - replace retained FreeType faces with bounded LRU probe-result caching - release non-trivial StaticLRU keys when clearing the cache - resolve the Texture Viewer close icon through the configured icon theme - add HTTP, TLS, UIWebView fan-out, font descriptor, and LRU regressions - update resource-refactor prerequisite and ownership planning documents --- ...resource_refactor_prerequisite_bugfixes.md | 16 +- ...ce_refactor_prerequisite_execution_plan.md | 14 +- ...ource_shared_ownership_stage0_inventory.md | 5 + include/eepp/core/lrucache.hpp | 43 ++- include/eepp/network/http.hpp | 90 ++++-- include/eepp/network/socket.hpp | 9 +- src/eepp/graphics/systemfontresolver.cpp | 132 +++++--- src/eepp/network/http.cpp | 281 ++++++++++++------ src/eepp/network/socket.cpp | 16 +- .../ssl/backend/mbedtls/mbedtlssocket.cpp | 48 +-- .../ssl/backend/mbedtls/mbedtlssocket.hpp | 3 +- src/eepp/network/tcplistener.cpp | 3 +- src/eepp/network/tcpsocket.cpp | 3 +- src/eepp/network/udpsocket.cpp | 6 +- src/eepp/ui/tools/uitextureviewer.cpp | 2 +- src/tests/unit_tests/http.cpp | 242 +++++++++++++++ .../resource_prerequisite_tests.cpp | 24 ++ .../unit_tests/systemfontresolver_tests.cpp | 41 +++ src/tests/unit_tests/uiwebview_tests.cpp | 176 ++++++++++- 19 files changed, 943 insertions(+), 211 deletions(-) diff --git a/.agent/plans/resource_refactor_prerequisite_bugfixes.md b/.agent/plans/resource_refactor_prerequisite_bugfixes.md index 5c599e7b7..9267f17f7 100644 --- a/.agent/plans/resource_refactor_prerequisite_bugfixes.md +++ b/.agent/plans/resource_refactor_prerequisite_bugfixes.md @@ -65,6 +65,17 @@ Regression coverage: - Running and queued variants complete/cancel without UAF under ASAN. - Callback re-entry does not deadlock. +Status: implemented. Shared-executor requests are registered before submission and retained by a +heap-backed operation. The operation destructor unregisters it both after execution and when the +external executor discards queued work. `Http::shutdown()` atomically rejects new work, +cancels registered requests, and waits without holding Pool or request-map locks. Callback-initiated +Pool clearing cannot wait for work queued behind that callback, so it requests cancellation and +lets shared operations defer destruction until the shared queue drains. `Request` cancellation is +shared and atomic across request copies. Focused ASAN coverage exercises queued memory, stream, and +file requests, running cancellation, concurrent Pool clearing, and Pool clearing from a callback +with another request queued behind it. All six focused HTTP tests pass under ASAN and unsuppressed +TSAN. + ### A3. Engine stops HTTP/resource producers too late Current behavior: @@ -86,8 +97,9 @@ Regression coverage: - Repeat Engine creation/destruction in the same test process. - Assert no callback touches the destroyed scene/factory and no singleton is recreated. -Status: Engine now clears pool-owned HTTP clients before scene/Graphics teardown. The complete -producer barrier remains pending A2 and A4 because shared-executor operations and static UI +Status: Engine now clears pool-owned HTTP clients before scene/Graphics teardown. Shared-executor +HTTP operations are covered by A2's explicit Pool barrier. The complete producer barrier remains +pending A4 because static UI deliveries do not yet have complete close/reject semantics. ### A4. UISceneNode static delivery queue lacks shutdown semantics diff --git a/.agent/plans/resource_refactor_prerequisite_execution_plan.md b/.agent/plans/resource_refactor_prerequisite_execution_plan.md index d8b2f5e73..5be79c0fc 100644 --- a/.agent/plans/resource_refactor_prerequisite_execution_plan.md +++ b/.agent/plans/resource_refactor_prerequisite_execution_plan.md @@ -264,6 +264,16 @@ Work-package exit criteria: - No shared-pool lambda depends on an untracked raw Http lifetime. - Http destruction provides a complete operation barrier without taking ownership of the executor. +Status: implemented. Shared-pool operations are heap-backed, registered before executor +submission, and unregister from their destructor even when queued work is discarded. Pool-managed +Http instances remain alive through callback completion, while stack/raw +instances use the destructor barrier. Pool clearing requests cancellation and waits outside the +Pool mutex. When clearing is initiated by a shared-pool callback, it defers the shared-operation +barrier because queued work may require the current executor thread; shared operations retain the +clients until that work drains. Cancellation state is atomic and shared by request copies. Focused +ASAN coverage for all three async output forms, running cancellation, and callback-initiated +clearing with queued work passes. All six focused HTTP tests also pass under unsuppressed TSAN. + ## 6. Work package 4: deterministic Engine shutdown Reorder shutdown only after HTTP and loader barriers are reliable. @@ -318,8 +328,8 @@ first, makes the selected context current, destroys scenes, explicitly discards TextLayout, releases high-level Graphics managers in dependency order, destroys shaders before Renderer, and only then destroys windows/contexts. A focused test covers two Engine lifecycles with a live UI scene, framebuffer, nine-patch, texture, font/layout cache, shaders, and pending batch. -The complete asynchronous-producer exit criterion remains pending Work Packages 3 and 5. -The complete ASAN unit suite passes: 746 tests passed and one opt-in visual test was skipped. +The complete asynchronous-producer exit criterion remains pending Work Package 5. +The complete ASAN unit suite passes: 749 tests passed and one opt-in visual test was skipped. ## 7. Work package 5: UISceneNode async delivery lifecycle diff --git a/.agent/plans/resource_shared_ownership_stage0_inventory.md b/.agent/plans/resource_shared_ownership_stage0_inventory.md index 150f2753e..d69c5d48f 100644 --- a/.agent/plans/resource_shared_ownership_stage0_inventory.md +++ b/.agent/plans/resource_shared_ownership_stage0_inventory.md @@ -43,6 +43,11 @@ The architecture contracts can proceed with these refinements: Members are destroyed in reverse declaration order, so the loader is destroyed last and can run against already-destroyed state. These are prerequisite bugs and should be fixed independently before the ownership refactor. + + Status: the HTTP Pool lock-order defect and shared-ThreadPool operation lifetime defect are + fixed. Pool clearing now establishes an operation barrier without owning the executor, including + callback-initiated clearing and executor-discarded queued work. The TextureAtlasLoader lifetime + defect is also fixed and covered by sanitizer-backed regression tests. 6. `isStateful()` is unusable as a shareability test. Every current Drawable inherits mutable color and position, and several classes reporting false mutate themselves or children during draw. 7. Stage 2 must migrate texture holders, loaders, ID-based construction, and queued batches in one diff --git a/include/eepp/core/lrucache.hpp b/include/eepp/core/lrucache.hpp index 9a7277560..8f0153bff 100644 --- a/include/eepp/core/lrucache.hpp +++ b/include/eepp/core/lrucache.hpp @@ -22,6 +22,8 @@ template > class DynamicLRU { private: + static_assert( Capacity > 0, "Capacity must be greater than zero for DynamicLRU" ); + using ListPair = std::pair; std::list mCacheList; UnorderedMap::iterator> mCacheMap; @@ -76,6 +78,7 @@ template > class StaticLRU { public: + static_assert( Capacity > 0, "Capacity must be greater than zero for StaticLRU" ); static_assert( Capacity < ( 1 << 16 ) - 2, "Capacity must be less than 65534 for StaticLRU" ); static constexpr std::size_t N = Capacity; static constexpr std::size_t HASH_SZ = 2 * N; // Load factor <= 0.5 @@ -94,7 +97,7 @@ class StaticLRU { std::optional get( const KeyT& key ) noexcept { const std::uint16_t idx = find_index( key ); - if ( idx == NONE ) + if ( idx >= N ) return std::nullopt; unlink( idx ); @@ -106,7 +109,7 @@ class StaticLRU { const std::uint16_t idx = find_index( key ); // Hit: update existing entry and move to front - if ( idx != NONE ) { + if ( idx < N ) { vals_[idx] = std::move( value ); unlink( idx ); push_front( idx ); @@ -121,9 +124,13 @@ class StaticLRU { } else { // Cache is full, evict the least recently used item (tail) target_idx = tail_; + if ( target_idx >= N ) + return; // O(1) removal from hash table using pre-stored position and a tombstone const std::uint16_t old_pos = hash_pos_[target_idx]; + if ( old_pos >= HASH_SZ ) + return; table_[old_pos].idx = DELETED_IDX; unlink( target_idx ); @@ -148,8 +155,10 @@ class StaticLRU { } void clear() { - for ( std::size_t i = 0; i < used_; ++i ) + for ( std::size_t i = 0; i < used_; ++i ) { + keys_[i] = KeyT{}; vals_[i] = ValueT{}; + } std::fill( table_.begin(), table_.end(), Entry{ NONE } ); std::fill( prev_.begin(), prev_.end(), NONE ); std::fill( next_.begin(), next_.end(), NONE ); @@ -177,20 +186,28 @@ class StaticLRU { alignas( 64 ) std::array hash_pos_{}; void unlink( std::uint16_t i ) noexcept { - if ( prev_[i] != NONE ) - next_[prev_[i]] = next_[i]; - if ( next_[i] != NONE ) - prev_[next_[i]] = prev_[i]; + if ( i >= N ) + return; + + const std::uint16_t previous = prev_[i]; + const std::uint16_t next = next_[i]; + if ( previous < N ) + next_[previous] = next; + if ( next < N ) + prev_[next] = previous; if ( head_ == i ) - head_ = next_[i]; + head_ = next; if ( tail_ == i ) - tail_ = prev_[i]; + tail_ = previous; } void push_front( std::uint16_t i ) noexcept { + if ( i >= N ) + return; + prev_[i] = NONE; next_[i] = head_; - if ( head_ != NONE ) + if ( head_ < N ) prev_[head_] = i; head_ = i; if ( tail_ == NONE ) @@ -209,7 +226,7 @@ class StaticLRU { // Empty slot means the key cannot be further down the probe chain. return NONE; } - if ( entry.idx != DELETED_IDX && keys_[entry.idx] == key ) { + if ( entry.idx < N && keys_[entry.idx] == key ) { // Found the key. return entry.idx; } @@ -226,6 +243,8 @@ template > class LRUCache { private: + static_assert( Capacity > 0, "Capacity must be greater than zero for LRUCache" ); + // Threshold for choosing the static implementation. If the estimated memory usage // is below this, the faster, allocation-free StaticLRU is used. static constexpr std::size_t STATIC_THRESHOLD_BYTES = 1 << 20; // 1 MB @@ -234,7 +253,7 @@ class LRUCache { constexpr std::size_t key_size = sizeof( KeyT ); constexpr std::size_t val_size = sizeof( ValueT ); constexpr std::size_t idx_size = sizeof( std::uint16_t ); - constexpr std::size_t entry_size = sizeof( typename StaticLRU<0, KeyT, ValueT>::Entry ); + constexpr std::size_t entry_size = sizeof( std::uint16_t ); constexpr std::size_t data_mem = ( key_size + val_size ) * Capacity; constexpr std::size_t lru_mem = ( idx_size * 2 ) * Capacity; // prev/next diff --git a/include/eepp/network/http.hpp b/include/eepp/network/http.hpp index 67d2d01fb..b3d313a24 100644 --- a/include/eepp/network/http.hpp +++ b/include/eepp/network/http.hpp @@ -1,6 +1,7 @@ #ifndef EE_NETWORKCHTTP_HPP #define EE_NETWORKCHTTP_HPP +#include #include #include #include @@ -13,6 +14,8 @@ #include #include #include +#include +#include #include namespace EE { namespace System { @@ -24,7 +27,7 @@ using namespace EE::System; namespace EE { namespace Network { /** @brief A HTTP client */ -class EE_API Http : NonCopyable { +class EE_API Http : NonCopyable, public std::enable_shared_from_this { public: /** @brief Define a HTTP response */ class EE_API Response { @@ -188,11 +191,11 @@ class EE_API Http : NonCopyable { /** @brief Enumerate the available states for a request */ enum Status { - Connected, ///< Connected to server. - Sent, ///< Request sent to the server. - HeaderReceived, ///< Header received. + Connected, ///< Connected to server. + Sent, ///< Request sent to the server. + HeaderReceived, ///< Header received. ContentReceived, ///< Content received. - Redirect, ///< A redirect has been handled + Redirect, ///< A redirect has been handled }; static std::string statusToString( Status status ); @@ -346,11 +349,12 @@ class EE_API Http : NonCopyable { /** Get the cancel callback */ const CancelCallback& getCancelCallback() const; - /** Cancels the current request if being processed */ + /** Cancels the current request if being processed. When resetCancelCallback is true, + * progress and cancellation callbacks are also released. */ void cancel( bool resetCancelCallback = false ); /** @return True if the current request was cancelled */ - const bool& isCancelled() const; + bool isCancelled() const; /** @return If requests a compressed response */ const bool& isCompressedResponse() const; @@ -400,11 +404,11 @@ class EE_API Http : NonCopyable { bool mFollowRedirect; ///< Follows redirect response codes bool mCompressedResponse; ///< Request compressed response bool mContinue; ///< Resume download - mutable bool mCancel; ///< Cancel state of current request - bool mVerbose{ false }; ///< Enable/Disable verbosity - ProgressCallback mProgressCallback; ///< Progress callback - CancelCallback mCancelCallback; ///< Cancel callback - unsigned int mMaxRedirections; ///< Maximum number of redirections allowed + std::shared_ptr> mCancel; ///< Cancel state shared by request copies + bool mVerbose{ false }; ///< Enable/Disable verbosity + ProgressCallback mProgressCallback; ///< Progress callback + CancelCallback mCancelCallback; ///< Cancel callback + unsigned int mMaxRedirections; ///< Maximum number of redirections allowed mutable unsigned int mRedirectionCount; ///< Number of redirections followed by the request URI mProxy; ///< Proxy information }; @@ -503,26 +507,31 @@ class EE_API Http : NonCopyable { Response downloadRequest( const Request& request, std::string writePath, Time timeout = Time::Zero ); - /** Definition of the async callback response */ + /** Definition of the async callback response. + * + * Pool/shared-owned clients are retained through operation completion. Destroying any other + * Http from a different thread cancels and joins its operations. A raw or stack Http must not + * be destroyed from one of its own callbacks. */ typedef std::function AsyncResponseCallback; - /** @brief Sends the request and creates a new thread, when got the response informs the result - ** to the callback. * This function does not lock the caller thread. + /** @brief Schedules the request asynchronously and passes the response to the callback. + ** This function does not lock the caller thread. ** @see sendRequest ** @return Unique Id of the request added */ Uint64 sendAsyncRequest( const AsyncResponseCallback& cb, const Http::Request& request, Time timeout = Time::Zero ); - /** @brief Sends the request and creates a new thread, when got the response informs the result - *to the callback. * This function does not lock the caller thread. + /** @brief Schedules the request asynchronously and passes the response to the callback. + ** This function does not lock the caller thread. + ** The caller must keep writeTo alive until the callback or an Http/Pool operation barrier. ** @see downloadRequest ** @return Unique Id of the request added */ Uint64 downloadAsyncRequest( const AsyncResponseCallback& cb, const Http::Request& request, IOStream& writeTo, Time timeout = Time::Zero ); - /** @brief Sends the request and creates a new thread, when got the response informs the result - *to the callback. * This function does not lock the caller thread. + /** @brief Schedules the request asynchronously and passes the response to the callback. + ** This function does not lock the caller thread. ** @see downloadRequest ** @return Unique Id of the request added */ Uint64 downloadAsyncRequest( const AsyncResponseCallback& cb, const Http::Request& request, @@ -626,7 +635,9 @@ class EE_API Http : NonCopyable { ~Pool(); - /** Clear all the HTTP Clients */ + /** Cancel and clear all HTTP clients. Outside an HTTP callback this is an operation + * barrier. From a shared-pool callback, completion is deferred to avoid waiting for work + * queued behind the current callback. */ void clear(); /** @return True if the client already exists in the pool @@ -709,7 +720,8 @@ class EE_API Http : NonCopyable { /** It will try to get the proxy from the environment variables. */ static URI getEnvProxyURI(); - /** Set the thread pool to consume for async requests, otherwise it will use its own */ + /** Set the externally owned thread pool used for async requests, otherwise each Http uses its + * own threads. Http cancellation and destruction do not stop or drain this pool. */ static void setThreadPool( std::shared_ptr pool ); private: @@ -734,6 +746,10 @@ class EE_API Http : NonCopyable { void cancel( bool resetCancelCallback = false ); + static AsyncRequest* current(); + + bool fromLocalPool() const { return mFromLocalPool; } + protected: friend class Http; Uint64 mId{ 0 }; @@ -741,11 +757,26 @@ class EE_API Http : NonCopyable { AsyncResponseCallback mCb; Http::Request mRequest; Time mTimeout; - bool mRunning; bool mStreamed; bool mStreamOwned; bool mFromLocalPool; IOStream* mStream; + + static thread_local AsyncRequest* sCurrent; + }; + + class SharedRequestOperation { + public: + SharedRequestOperation( Http& http, std::shared_ptr request ); + + ~SharedRequestOperation(); + + void run(); + + private: + Http& mHttp; + std::shared_ptr mOwner; + std::shared_ptr mRequest; }; class HttpConnection { @@ -797,10 +828,21 @@ class EE_API Http : NonCopyable { bool mHostSolved; std::atomic mShuttingDown{ false }; URI mProxy; - Mutex mCurRequestsMutex; + std::mutex mRequestsMutex; + std::condition_variable mRequestsComplete; std::unordered_map mCurRequests; + std::unordered_map> mSharedRequests; - void removeAsyncRequest( AsyncRequest* req ); + bool removeAsyncRequest( AsyncRequest* req ); + + bool registerSharedRequest( const std::shared_ptr& request ); + + void completeSharedRequest( const std::shared_ptr& request ); + + bool scheduleSharedRequest( const std::shared_ptr& threadPool, + const std::shared_ptr& request ); + + void shutdown( bool waitForSharedRequests = true ); Request prepareFields( const Http::Request& request ); diff --git a/include/eepp/network/socket.hpp b/include/eepp/network/socket.hpp index e5a8257d4..a7603a5ad 100644 --- a/include/eepp/network/socket.hpp +++ b/include/eepp/network/socket.hpp @@ -65,10 +65,11 @@ class EE_API Socket : NonCopyable { ** @return The internal (OS-specific) handle of the socket */ SocketHandle getHandle() const; - /** @brief Create the internal representation of the socket - /// - ** This function can only be accessed by derived classes. */ - void create(); + /** @brief Create the internal representation of the socket + /// + ** This function can only be accessed by derived classes. + ** @return True when a valid socket exists, false when socket creation failed. */ + bool create(); /** @brief Create the internal representation of the socket from a socket handle ** This function can only be accessed by derived classes. diff --git a/src/eepp/graphics/systemfontresolver.cpp b/src/eepp/graphics/systemfontresolver.cpp index 8d97cb569..0c73ba0ad 100644 --- a/src/eepp/graphics/systemfontresolver.cpp +++ b/src/eepp/graphics/systemfontresolver.cpp @@ -5,6 +5,7 @@ #undef Rect #endif +#include #include #include #include @@ -52,42 +53,93 @@ using namespace EE::System; +namespace EE::Graphics::SystemFontResolverDetail { + +enum class FontProbeKind : Uint8 { Codepoint, SfntTable }; + +struct FontProbeKey { + std::string path; + FT_ULong value; + FontProbeKind kind; + + bool operator==( const FontProbeKey& other ) const { + return value == other.value && kind == other.kind && path == other.path; + } +}; + +} // namespace EE::Graphics::SystemFontResolverDetail + +namespace std { + +template <> struct hash { + std::size_t + operator()( const EE::Graphics::SystemFontResolverDetail::FontProbeKey& key ) const noexcept { + return hashCombine( std::hash{}( key.path ), + std::hash{}( key.value ), + static_cast( key.kind ) ); + } +}; + +} // namespace std + namespace { +using FontProbeKey = EE::Graphics::SystemFontResolverDetail::FontProbeKey; +using FontProbeKind = EE::Graphics::SystemFontResolverDetail::FontProbeKind; + struct FreeTypeState { + static constexpr std::size_t MaxCachedProbes = 4096; + FT_Library library{ nullptr }; - EE::UnorderedMap faceCache; Mutex mutex; + EE::LRUCache probeCache; FreeTypeState() { FT_Init_FreeType( &library ); } ~FreeTypeState() { Lock lock( mutex ); - - for ( auto& pair : faceCache ) { - FT_Done_Face( pair.second ); - } - - if ( library ) { + if ( library ) FT_Done_FreeType( library ); - } } - FT_Face getFace( const std::string& path ) { + bool containsCodepoint( const std::string& path, EE::Uint32 codepoint ) { Lock lock( mutex ); + FontProbeKey key{ path, codepoint, FontProbeKind::Codepoint }; + if ( auto cached = probeCache.get( key ) ) + return *cached; - auto it = faceCache.find( path ); - if ( it != faceCache.end() ) { - return it->second; + FT_Face face{ nullptr }; + bool contains = false; + if ( library && FT_New_Face( library, path.c_str(), 0, &face ) == 0 ) { + contains = FT_Get_Char_Index( face, codepoint ) != 0; + FT_Done_Face( face ); } - FT_Face face; - if ( FT_New_Face( library, path.c_str(), 0, &face ) == 0 ) { - faceCache[path] = face; - return face; + probeCache.put( std::move( key ), contains ); + return contains; + } + + bool hasSfntTable( const std::string& path, FT_ULong tag ) { + Lock lock( mutex ); + FontProbeKey key{ path, tag, FontProbeKind::SfntTable }; + if ( auto cached = probeCache.get( key ) ) + return *cached; + + FT_Face face{ nullptr }; + bool found = false; + if ( library && FT_New_Face( library, path.c_str(), 0, &face ) == 0 ) { + FT_ULong length = 0; + found = FT_Load_Sfnt_Table( face, tag, 0, nullptr, &length ) == 0 && length > 0; + FT_Done_Face( face ); } - return nullptr; + probeCache.put( std::move( key ), found ); + return found; + } + + void clearProbeCache() { + Lock lock( mutex ); + probeCache.clear(); } }; @@ -106,17 +158,19 @@ void destroyFTState() { gFtState.reset(); } +void clearFTProbeCache() { + std::shared_ptr state; + { + Lock lock( gStateInitMutex ); + state = gFtState; + } + if ( state ) + state->clearProbeCache(); +} + bool fontHasSfntTable( const std::string& path, FT_ULong tag ) { std::shared_ptr state = getFTState(); - if ( !state ) - return false; - - FT_Face face = state->getFace( path ); - if ( !face ) - return false; - - FT_ULong length = 0; - return FT_Load_Sfnt_Table( face, tag, 0, nullptr, &length ) == 0 && length > 0; + return state && state->hasSfntTable( path, tag ); } bool fontHasSvgTable( const std::string& path ) { @@ -147,13 +201,16 @@ bool SystemFontResolver::isEnabled() { } void SystemFontResolver::invalidateCache() { - Lock lock( mMutex ); - mResolveCache.clear(); - mGenericCache.clear(); - mCodepointFallbackCache.clear(); - mFontList.clear(); - mGenericFallbacks.clear(); - mFontListPopulated = false; + { + Lock lock( mMutex ); + mResolveCache.clear(); + mGenericCache.clear(); + mCodepointFallbackCache.clear(); + mFontList.clear(); + mGenericFallbacks.clear(); + mFontListPopulated = false; + } + clearFTProbeCache(); } static std::string normalizeFamily( const std::string& family ) { @@ -436,14 +493,7 @@ FontDesc SystemFontResolver::getFallbackForCodepoint( Uint32 codepoint, FontWeig bool SystemFontResolver::fontContainsCodepoint( const std::string& path, Uint32 codepoint ) { std::shared_ptr state = getFTState(); - if ( !state ) - return false; - - FT_Face face = state->getFace( path ); - if ( !face ) - return false; - - return FT_Get_Char_Index( face, codepoint ) != 0; + return state && state->containsCodepoint( path, codepoint ); } void SystemFontResolver::populateGenericFallbacks() const { diff --git a/src/eepp/network/http.cpp b/src/eepp/network/http.cpp index 570dfbc74..0fb5aeac3 100644 --- a/src/eepp/network/http.cpp +++ b/src/eepp/network/http.cpp @@ -108,7 +108,7 @@ Http::Request::Request( const std::string& uri, Method method, const std::string mFollowRedirect( followRedirect ), mCompressedResponse( compressedResponse ), mContinue( false ), - mCancel( false ), + mCancel( std::make_shared>( false ) ), mMaxRedirections( 10 ), mRedirectionCount( 0 ) { setMethod( method ); @@ -215,14 +215,15 @@ const Http::Request::CancelCallback& Http::Request::getCancelCallback() const { } void Http::Request::cancel( bool resetCancelCallback ) { - mCancel = true; - setProgressCallback( {} ); - if ( resetCancelCallback ) + mCancel->store( true, std::memory_order_release ); + if ( resetCancelCallback ) { + setProgressCallback( {} ); setCancelCallback( {} ); + } } -const bool& Http::Request::isCancelled() const { - return mCancel; +bool Http::Request::isCancelled() const { + return mCancel->load( std::memory_order_acquire ); } std::string Http::Request::prepareTunnel( const Http& http ) { @@ -625,6 +626,12 @@ void Http::Response::parseFields( std::istream& in ) { static Http::Pool sGlobalHttpPool = Http::Pool(); static std::shared_ptr sGlobalThreadPool = nullptr; +static std::mutex sGlobalThreadPoolMutex; + +static std::shared_ptr getGlobalThreadPool() { + std::lock_guard lock( sGlobalThreadPoolMutex ); + return sGlobalThreadPool; +} Http::Response Http::request( const URI& uri, Request::Method method, const Time& timeout, const Http::Request::ProgressCallback& progressCallback, @@ -709,23 +716,7 @@ Http::Http( const std::string& host, unsigned short port, bool useSSL, URI proxy } Http::~Http() { - mShuttingDown = true; - - { - Lock l( mCurRequestsMutex ); - for ( auto [_, req] : mCurRequests ) - req->cancel(); - } - - { - Lock l( mThreadsMutex ); - // First we wait to finish any request pending - for ( auto& thread : mThreads ) { - thread->cancel(); - thread->wait(); - eeDelete( thread ); - } - } + shutdown(); // Then we destroy the last open connection HttpConnection* connection = mConnection; @@ -796,6 +787,8 @@ Http::Response Http::sendRequest( const Http::Request& request, Time timeout ) { static bool sendProgress( const Http& http, const Http::Request& request, const Http::Response& response, const Http::Request::Status& status, const std::size_t& totalBytes, const std::size_t& currentBytes ) { + if ( request.isCancelled() ) + return false; if ( request.getProgressCallback() ) return request.getProgressCallback()( http, request, response, status, totalBytes, currentBytes ); @@ -978,7 +971,7 @@ Http::Response Http::downloadRequest( const Http::Request& request, IOStream& wr requestStr.c_str(), requestStr.size() ) == Socket::Done ) { if ( !request.isCancelled() && !sendProgress( *this, request, received, Request::Sent, 0, 0 ) ) { - request.mCancel = true; + request.mCancel->store( true, std::memory_order_release ); } // Wait for the server's response @@ -1089,7 +1082,8 @@ Http::Response Http::downloadRequest( const Http::Request& request, IOStream& wr !sendProgress( *this, request, received, Request::Redirect, contentLength, currentTotalBytes ) ) { - request.mCancel = true; + request.mCancel->store( true, + std::memory_order_release ); } else { Http::Request newRequest( request ); newRequest.setUri( uri.getPathAndQuery() ); @@ -1129,7 +1123,7 @@ Http::Response Http::downloadRequest( const Http::Request& request, IOStream& wr if ( !request.isCancelled() && !sendProgress( *this, request, received, Request::HeaderReceived, contentLength, 0 ) ) { - request.mCancel = true; + request.mCancel->store( true, std::memory_order_release ); } // Move the response body bytes already read into the socket buffer. @@ -1155,7 +1149,7 @@ Http::Response Http::downloadRequest( const Http::Request& request, IOStream& wr if ( !request.isCancelled() && !sendProgress( *this, request, received, Request::ContentReceived, contentLength, currentTotalBytes ) ) { - request.mCancel = true; + request.mCancel->store( true, std::memory_order_release ); break; } @@ -1241,10 +1235,12 @@ Http::Response Http::downloadRequest( const Http::Request& request, std::string } void Http::setThreadPool( std::shared_ptr pool ) { + std::lock_guard lock( sGlobalThreadPoolMutex ); sGlobalThreadPool = pool; } std::atomic Http::AsyncRequest::IdCounter = 1; +thread_local Http::AsyncRequest* Http::AsyncRequest::sCurrent = nullptr; Http::AsyncRequest::AsyncRequest( Uint64 id, Http* http, const Http::AsyncResponseCallback& cb, Http::Request request, Time timeout, bool fromLocalPool ) : @@ -1253,7 +1249,6 @@ Http::AsyncRequest::AsyncRequest( Uint64 id, Http* http, const Http::AsyncRespon mCb( cb ), mRequest( request ), mTimeout( timeout ), - mRunning( true ), mStreamed( false ), mStreamOwned( false ), mFromLocalPool( fromLocalPool ), @@ -1267,7 +1262,6 @@ Http::AsyncRequest::AsyncRequest( Uint64 id, Http* http, const Http::AsyncRespon mCb( cb ), mRequest( request ), mTimeout( timeout ), - mRunning( true ), mStreamed( true ), mStreamOwned( false ), mFromLocalPool( fromLocalPool ), @@ -1281,7 +1275,6 @@ Http::AsyncRequest::AsyncRequest( Uint64 id, Http* http, const Http::AsyncRespon mCb( cb ), mRequest( request ), mTimeout( timeout ), - mRunning( true ), mStreamed( true ), mStreamOwned( true ), mFromLocalPool( fromLocalPool ), @@ -1297,6 +1290,9 @@ void Http::AsyncRequest::cancel( bool resetCancelCallback ) { } void Http::AsyncRequest::run() { + AsyncRequest* previousRequest = sCurrent; + sCurrent = this; + Http::Response response = mStreamed ? mHttp->downloadRequest( mRequest, *mStream, mTimeout ) : mHttp->sendRequest( mRequest, mTimeout ); @@ -1307,27 +1303,120 @@ void Http::AsyncRequest::run() { eeSAFE_DELETE( mStream ); } - mRunning = false; - - if ( mFromLocalPool && !mHttp->mShuttingDown ) { - mHttp->removeAsyncRequest( this ); + if ( mFromLocalPool && mHttp->removeAsyncRequest( this ) ) { auto me = this; + sCurrent = previousRequest; eeSAFE_DELETE( me ); + return; } + + sCurrent = previousRequest; } -void Http::removeAsyncRequest( AsyncRequest* req ) { +Http::AsyncRequest* Http::AsyncRequest::current() { + return sCurrent; +} + +Http::SharedRequestOperation::SharedRequestOperation( Http& http, + std::shared_ptr request ) : + mHttp( http ), mOwner( http.weak_from_this().lock() ), mRequest( std::move( request ) ) {} + +Http::SharedRequestOperation::~SharedRequestOperation() { + mHttp.completeSharedRequest( mRequest ); +} + +void Http::SharedRequestOperation::run() { + mRequest->run(); +} + +bool Http::removeAsyncRequest( AsyncRequest* req ) { { - Lock l( mCurRequestsMutex ); + std::lock_guard lock( mRequestsMutex ); mCurRequests.erase( req->id() ); } + Lock l( mThreadsMutex ); + if ( mShuttingDown.load( std::memory_order_acquire ) ) + return false; + auto found = std::find( mThreads.begin(), mThreads.end(), req ); + if ( found == mThreads.end() ) + return false; + mThreads.erase( found ); + return true; +} + +bool Http::registerSharedRequest( const std::shared_ptr& request ) { + std::lock_guard lock( mRequestsMutex ); + if ( mShuttingDown.load( std::memory_order_acquire ) ) + return false; + mCurRequests[request->id()] = request.get(); + mSharedRequests[request->id()] = request; + return true; +} + +void Http::completeSharedRequest( const std::shared_ptr& request ) { + { + std::lock_guard lock( mRequestsMutex ); + mCurRequests.erase( request->id() ); + mSharedRequests.erase( request->id() ); + } + mRequestsComplete.notify_all(); +} + +bool Http::scheduleSharedRequest( const std::shared_ptr& threadPool, + const std::shared_ptr& request ) { + if ( !registerSharedRequest( request ) ) { + request->cancel(); + return false; + } + + // The executor is externally owned and may discard queued work. The operation reports + // completion when the queued function releases it, whether or not run() was called. + auto operation = std::make_shared( *this, request ); + threadPool->run( [operation] { operation->run(); } ); + return true; +} + +void Http::shutdown( bool waitForSharedRequests ) { + std::vector> sharedRequests; + { + std::lock_guard lock( mRequestsMutex ); + mShuttingDown.store( true, std::memory_order_release ); + sharedRequests.reserve( mSharedRequests.size() ); + for ( const auto& [_, request] : mSharedRequests ) + sharedRequests.emplace_back( request ); + } + + for ( const auto& request : sharedRequests ) + request->cancel(); + + std::vector threads; { Lock l( mThreadsMutex ); - auto found = std::find( mThreads.begin(), mThreads.end(), req ); - if ( found != mThreads.end() ) - mThreads.erase( found ); + threads.swap( mThreads ); } + for ( auto* thread : threads ) { + thread->cancel(); + thread->wait(); + { + std::lock_guard lock( mRequestsMutex ); + mCurRequests.erase( thread->id() ); + } + eeDelete( thread ); + } + if ( !waitForSharedRequests ) + return; + + AsyncRequest* currentRequest = AsyncRequest::current(); + std::unique_lock lock( mRequestsMutex ); + mRequestsComplete.wait( lock, [this, currentRequest] { + if ( currentRequest && currentRequest->mHttp == this && !currentRequest->mFromLocalPool ) { + return mSharedRequests.empty() || + ( mSharedRequests.size() == 1 && + mSharedRequests.begin()->second.get() == currentRequest ); + } + return mSharedRequests.empty(); + } ); } Http::Request Http::prepareFields( const Http::Request& request ) { @@ -1388,7 +1477,7 @@ bool Http::isProxied() const { } bool Http::setCancelRequest( Uint64 reqId, bool resetCancelCallback ) { - Lock l( mCurRequestsMutex ); + std::lock_guard lock( mRequestsMutex ); auto found = mCurRequests.find( reqId ); if ( found != mCurRequests.end() ) { found->second->cancel( resetCancelCallback ); @@ -1469,30 +1558,25 @@ Uint64 Http::sendAsyncRequest( const Http::AsyncResponseCallback& cb, const Http emscripten_async_wget2_got_error_data, NULL ); return id; #else - if ( sGlobalThreadPool ) { - sGlobalThreadPool->run( [this, cb, request, timeout, id] { - AsyncRequest asyncRequest( id, this, cb, request, timeout, false ); - { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = &asyncRequest; - } - asyncRequest.run(); - { - Lock l( mCurRequestsMutex ); - mCurRequests.erase( id ); - } - } ); + auto threadPool = getGlobalThreadPool(); + if ( threadPool ) { + auto asyncRequest = std::make_shared( id, this, cb, request, timeout, false ); + scheduleSharedRequest( threadPool, asyncRequest ); return id; } AsyncRequest* thread = eeNew( AsyncRequest, ( id, this, cb, request, timeout, true ) ); { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = thread; - } - thread->launch(); - { - Lock l( mThreadsMutex ); + Lock threadsLock( mThreadsMutex ); + { + std::lock_guard requestsLock( mRequestsMutex ); + if ( mShuttingDown.load( std::memory_order_acquire ) ) { + eeDelete( thread ); + return id; + } + mCurRequests[id] = thread; + } mThreads.push_back( thread ); + thread->launch(); } return id; #endif @@ -1514,30 +1598,26 @@ Uint64 Http::downloadAsyncRequest( const Http::AsyncResponseCallback& cb, emscripten_async_wget2_got_error_data, NULL ); return id; #else - if ( sGlobalThreadPool ) { - sGlobalThreadPool->run( [this, cb, request, &writeTo, timeout, id] { - AsyncRequest asyncRequest( id, this, cb, request, writeTo, timeout, false ); - { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = &asyncRequest; - } - asyncRequest.run(); - { - Lock l( mCurRequestsMutex ); - mCurRequests.erase( id ); - } - } ); + auto threadPool = getGlobalThreadPool(); + if ( threadPool ) { + auto asyncRequest = + std::make_shared( id, this, cb, request, writeTo, timeout, false ); + scheduleSharedRequest( threadPool, asyncRequest ); return id; } AsyncRequest* thread = eeNew( AsyncRequest, ( id, this, cb, request, writeTo, timeout, true ) ); { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = thread; - } - thread->launch(); - { - Lock l( mThreadsMutex ); + Lock threadsLock( mThreadsMutex ); + { + std::lock_guard requestsLock( mRequestsMutex ); + if ( mShuttingDown.load( std::memory_order_acquire ) ) { + eeDelete( thread ); + return id; + } + mCurRequests[id] = thread; + } mThreads.push_back( thread ); + thread->launch(); } return id; #endif @@ -1558,31 +1638,27 @@ Uint64 Http::downloadAsyncRequest( const Http::AsyncResponseCallback& cb, emscripten_async_wget2_got_file, emscripten_async_wget2_got_error_file, NULL ); #else - if ( sGlobalThreadPool ) { - sGlobalThreadPool->run( [this, cb, request, writePath, timeout, id] { - AsyncRequest asyncRequest( id, this, cb, request, writePath, timeout, false ); - { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = &asyncRequest; - } - asyncRequest.run(); - { - Lock l( mCurRequestsMutex ); - mCurRequests.erase( id ); - } - } ); + auto threadPool = getGlobalThreadPool(); + if ( threadPool ) { + auto asyncRequest = + std::make_shared( id, this, cb, request, writePath, timeout, false ); + scheduleSharedRequest( threadPool, asyncRequest ); return id; } AsyncRequest* thread = eeNew( AsyncRequest, ( id, this, cb, request, writePath, timeout, true ) ); { - Lock l( mCurRequestsMutex ); - mCurRequests[id] = thread; - } - thread->launch(); - { - Lock l( mThreadsMutex ); + Lock threadsLock( mThreadsMutex ); + { + std::lock_guard requestsLock( mRequestsMutex ); + if ( mShuttingDown.load( std::memory_order_acquire ) ) { + eeDelete( thread ); + return id; + } + mCurRequests[id] = thread; + } mThreads.push_back( thread ); + thread->launch(); } #endif return id; @@ -1690,8 +1766,15 @@ void Http::Pool::clear() { Lock l( mMutex ); https.swap( mHttps ); } - // Http destruction joins local request threads and callbacks can re-enter the global pool. - // Never run either operation while holding the pool mutex. + // Requests may keep their Http alive while running on the shared ThreadPool. Establish the + // operation barrier explicitly before releasing the Pool's ownership. Callbacks can re-enter + // the global Pool, so never wait while holding the Pool mutex. A shared-pool callback also + // cannot wait for requests queued behind itself; their shared operations retain each Http until + // drained. + AsyncRequest* currentRequest = AsyncRequest::current(); + const bool waitForSharedRequests = !currentRequest || currentRequest->fromLocalPool(); + for ( const auto& [_, http] : https ) + http->shutdown( waitForSharedRequests ); https.clear(); } diff --git a/src/eepp/network/socket.cpp b/src/eepp/network/socket.cpp index 9155cab3a..433b59070 100644 --- a/src/eepp/network/socket.cpp +++ b/src/eepp/network/socket.cpp @@ -1,3 +1,5 @@ +#include +#include #include #include #include @@ -28,12 +30,24 @@ SocketHandle Socket::getHandle() const { return mSocket; } -void Socket::create() { +bool Socket::create() { // Don't create the socket if it already exists if ( mSocket == Private::SocketImpl::invalidSocket() ) { SocketHandle handle = socket( PF_INET, mType == Tcp ? SOCK_STREAM : SOCK_DGRAM, 0 ); + if ( handle == Private::SocketImpl::invalidSocket() ) { +#if EE_PLATFORM == EE_PLATFORM_WIN + Log::error( "Failed to create %s socket (Winsock error %d)", + mType == Tcp ? "TCP" : "UDP", WSAGetLastError() ); +#else + const int errorCode = errno; + Log::error( "Failed to create %s socket: %s (errno %d)", mType == Tcp ? "TCP" : "UDP", + std::strerror( errorCode ), errorCode ); +#endif + return false; + } create( handle ); } + return true; } void Socket::create( SocketHandle handle ) { diff --git a/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.cpp b/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.cpp index 0513d3791..9117ba4ff 100644 --- a/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.cpp +++ b/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.cpp @@ -59,6 +59,7 @@ bool MbedTLSSocket::end() { MbedTLSSocket::MbedTLSSocket( SSLSocket* socket ) : SSLSocketImpl( socket ), + mInitialized( false ), mConnected( false ), mSessionOwner( false ), mStatus( Socket::Disconnected ), @@ -110,9 +111,9 @@ int MbedTLSSocket::bio_recv( void* ctx, unsigned char* buf, size_t len ) { Socket::Status MbedTLSSocket::connect( const IpAddress& /*remoteAddress*/, unsigned short /*remotePort*/, Time timeout ) { - if ( mConnected ) { - disconnect(); - } + // A previous handshake may have initialized TLS state without ever reaching Connected. + // Always release it before starting a new handshake. + disconnect(); bool isBlocking = mSSLSocket->isBlocking(); if ( isBlocking && timeout != Time::Zero ) { @@ -129,12 +130,15 @@ Socket::Status MbedTLSSocket::connect( const IpAddress& /*remoteAddress*/, mbedtls_ssl_config_init( &mSSLConfig ); mbedtls_ctr_drbg_init( &mCtrDrbg ); mbedtls_entropy_init( &mEntropy ); + mInitialized = true; ret = mbedtls_ctr_drbg_seed( &mCtrDrbg, mbedtls_entropy_func, &mEntropy, NULL, 0 ); if ( ret != 0 ) { Log::error( " failed\n ! mbedtls_ctr_drbg_seed returned an error: %d", ret ); - return Socket::Error; + disconnect(); + mStatus = Socket::Error; + return mStatus; } mbedtls_ssl_config_defaults( &mSSLConfig, MBEDTLS_SSL_IS_CLIENT, MBEDTLS_SSL_TRANSPORT_STREAM, @@ -167,18 +171,22 @@ Socket::Status MbedTLSSocket::connect( const IpAddress& /*remoteAddress*/, mSSLSocket->setSendTimeout( Time::Zero ); } - if ( mStatus == Socket::Error ) + if ( mStatus == Socket::Error ) { + disconnect(); + mStatus = Socket::Error; return mStatus; + } mSSLSession = (mbedtls_ssl_session*)eeMalloc( sizeof( mbedtls_ssl_session ) ); - mbedtls_ssl_session_init( mSSLSession ); - ret = mbedtls_ssl_get_session( &mSSLContext, mSSLSession ); - if ( ret ) { - if ( ret != MBEDTLS_ERR_SSL_ALLOC_FAILED ) + if ( mSSLSession ) { + mbedtls_ssl_session_init( mSSLSession ); + ret = mbedtls_ssl_get_session( &mSSLContext, mSSLSession ); + if ( ret ) { mbedtls_ssl_session_free( mSSLSession ); - eeSAFE_FREE( mSSLSession ); - } else { - mSessionOwner = true; + eeSAFE_FREE( mSSLSession ); + } else { + mSessionOwner = true; + } } } else { MbedTLSSocket* oldSocket = @@ -193,19 +201,21 @@ Socket::Status MbedTLSSocket::connect( const IpAddress& /*remoteAddress*/, } void MbedTLSSocket::disconnect() { - if ( !mConnected ) - return; - - mbedtls_ssl_free( &mSSLContext ); - mbedtls_ssl_config_free( &mSSLConfig ); - mbedtls_ctr_drbg_free( &mCtrDrbg ); - mbedtls_entropy_free( &mEntropy ); + if ( mInitialized ) { + mbedtls_ssl_free( &mSSLContext ); + mbedtls_ssl_config_free( &mSSLConfig ); + mbedtls_ctr_drbg_free( &mCtrDrbg ); + mbedtls_entropy_free( &mEntropy ); + mInitialized = false; + } if ( mSessionOwner && NULL != mSSLSession ) { mbedtls_ssl_session_free( mSSLSession ); eeSAFE_FREE( mSSLSession ); } + mConnected = false; + mSessionOwner = false; mStatus = Socket::Disconnected; } diff --git a/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.hpp b/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.hpp index c00938d00..ea3f6a950 100644 --- a/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.hpp +++ b/src/eepp/network/ssl/backend/mbedtls/mbedtlssocket.hpp @@ -5,10 +5,10 @@ #ifdef EE_MBEDTLS -#include #include #include #include +#include #include namespace EE { namespace Network { namespace SSL { @@ -40,6 +40,7 @@ class EE_API MbedTLSSocket : public SSLSocketImpl { mbedtls_ctr_drbg_context mCtrDrbg; mbedtls_ssl_context mSSLContext; mbedtls_ssl_config mSSLConfig; + bool mInitialized; bool mConnected; bool mSessionOwner; Socket::Status mStatus; diff --git a/src/eepp/network/tcplistener.cpp b/src/eepp/network/tcplistener.cpp index 91d05168f..43c379102 100644 --- a/src/eepp/network/tcplistener.cpp +++ b/src/eepp/network/tcplistener.cpp @@ -27,7 +27,8 @@ Socket::Status TcpListener::listen( unsigned short port, const IpAddress& addres close(); // Create the internal socket if it doesn't exist - create(); + if ( !create() ) + return Error; // Check if the address is valid if ( ( address == IpAddress::None ) || ( address == IpAddress::Broadcast ) ) diff --git a/src/eepp/network/tcpsocket.cpp b/src/eepp/network/tcpsocket.cpp index 58d8d589a..81b81b0fa 100644 --- a/src/eepp/network/tcpsocket.cpp +++ b/src/eepp/network/tcpsocket.cpp @@ -91,7 +91,8 @@ Socket::Status TcpSocket::connect( const IpAddress& remoteAddress, unsigned shor disconnect(); // Create the internal socket if it doesn't exist - create(); + if ( !create() ) + return Error; // Create the remote address sockaddr_in address = diff --git a/src/eepp/network/udpsocket.cpp b/src/eepp/network/udpsocket.cpp index 27c2282d0..7f533b332 100644 --- a/src/eepp/network/udpsocket.cpp +++ b/src/eepp/network/udpsocket.cpp @@ -31,7 +31,8 @@ Socket::Status UdpSocket::bind( unsigned short port, const IpAddress& address ) close(); // Create the internal socket if it doesn't exist - create(); + if ( !create() ) + return Error; // Check if the address is valid if ( ( address == IpAddress::None ) || ( address == IpAddress::Broadcast ) ) @@ -55,7 +56,8 @@ void UdpSocket::unbind() { Socket::Status UdpSocket::send( const void* data, std::size_t size, const IpAddress& remoteAddress, unsigned short remotePort ) { // Create the internal socket if it doesn't exist - create(); + if ( !create() ) + return Error; // Make sure that all the data will fit in one datagram if ( size > MaxDatagramSize ) { diff --git a/src/eepp/ui/tools/uitextureviewer.cpp b/src/eepp/ui/tools/uitextureviewer.cpp index c4e998f72..4d861fbf8 100644 --- a/src/eepp/ui/tools/uitextureviewer.cpp +++ b/src/eepp/ui/tools/uitextureviewer.cpp @@ -41,7 +41,7 @@ void UITextureViewer::init() { - + )xml", this ); diff --git a/src/tests/unit_tests/http.cpp b/src/tests/unit_tests/http.cpp index 79c780439..13637b0ec 100644 --- a/src/tests/unit_tests/http.cpp +++ b/src/tests/unit_tests/http.cpp @@ -8,8 +8,12 @@ #include #include +#include #include #include +#include +#include +#include #if EE_PLATFORM != EE_PLATFORM_WIN #include @@ -20,6 +24,37 @@ using namespace EE; using namespace EE::Network; +namespace { + +struct ThreadGate { + void enterAndWait() { + std::unique_lock lock( mutex ); + entered = true; + condition.notify_all(); + condition.wait( lock, [this] { return released; } ); + } + + bool waitUntilEntered() { + std::unique_lock lock( mutex ); + return condition.wait_for( lock, std::chrono::seconds( 5 ), [this] { return entered; } ); + } + + void release() { + { + std::lock_guard lock( mutex ); + released = true; + } + condition.notify_all(); + } + + std::mutex mutex; + std::condition_variable condition; + bool entered{ false }; + bool released{ false }; +}; + +} // namespace + #if EE_PLATFORM != EE_PLATFORM_WIN namespace { @@ -81,6 +116,42 @@ UTEST( Http, responseHeaderLineLargerThanReceiveBuffer ) { EXPECT_TRUE( response.getBody() == "hello" ); } +UTEST( Http, failedTlsHandshakesReleaseConnectionState ) { + if ( !SSL::SSLSocket::isSupported() ) + return; + + constexpr int Attempts = 16; + TcpListener listener; + ASSERT_EQ( listener.listen( Socket::AnyPort, IpAddress::LocalHost ), Socket::Done ); + + std::atomic acceptedConnections{ 0 }; + std::thread server( [&] { + for ( int i = 0; i < Attempts; ++i ) { + TcpSocket client; + if ( listener.accept( client ) != Socket::Done ) + return; + + char clientHello[1024]; + std::size_t received = 0; + if ( client.receive( clientHello, sizeof( clientHello ), received ) != Socket::Done ) + return; + acceptedConnections.fetch_add( 1, std::memory_order_release ); + client.disconnect(); + } + } ); + + Http http( "127.0.0.1", listener.getLocalPort(), true ); + for ( int i = 0; i < Attempts; ++i ) { + Http::Request request( "/", Http::Request::Get, "", false, false ); + Http::Response response = http.sendRequest( request, Seconds( 2 ) ); + EXPECT_EQ( response.getStatus(), Http::Response::ConnectionFailed ); + } + + server.join(); + listener.close(); + EXPECT_EQ( acceptedConnections.load( std::memory_order_acquire ), Attempts ); +} + UTEST( Http, poolClearAllowsCallbackReentry ) { Http::setThreadPool( nullptr ); Http::Pool::getGlobal().clear(); @@ -168,6 +239,177 @@ UTEST( Http, poolClearAllowsCallbackReentry ) { EXPECT_TRUE( callbackResponseOk ); } +UTEST( Http, sharedThreadPoolJoinsQueuedRequestsOnPoolClear ) { + Http::setThreadPool( nullptr ); + Http::Pool::getGlobal().clear(); + + auto threadPool = System::ThreadPool::createShared( 1 ); + ThreadGate workerGate; + threadPool->run( [&] { workerGate.enterAndWait(); } ); + ASSERT_TRUE( workerGate.waitUntilEntered() ); + Http::setThreadPool( threadPool ); + + const URI uri( "http://127.0.0.1:1/" ); + auto http = Http::Pool::getGlobal().get( uri ); + System::IOStreamString stream; + const std::string outputPath = + System::Sys::getTempPath() + "eepp-http-shared-pool-shutdown.tmp"; + System::FileSystem::fileRemove( outputPath ); + std::atomic callbacks{ 0 }; + auto callback = [&]( const Http&, Http::Request&, Http::Response& ) { callbacks++; }; + + http->sendAsyncRequest( callback, Http::Request( "/memory" ), Seconds( 5 ) ); + http->downloadAsyncRequest( callback, Http::Request( "/stream" ), stream, Seconds( 5 ) ); + http->downloadAsyncRequest( callback, Http::Request( "/file" ), outputPath, Seconds( 5 ) ); + http.reset(); + + std::atomic clearCompleted{ false }; + std::thread clearThread( [&] { + Http::Pool::getGlobal().clear(); + clearCompleted.store( true, std::memory_order_release ); + } ); + std::this_thread::sleep_for( std::chrono::milliseconds( 50 ) ); + EXPECT_FALSE( clearCompleted.load( std::memory_order_acquire ) ); + + workerGate.release(); + clearThread.join(); + EXPECT_TRUE( clearCompleted.load( std::memory_order_acquire ) ); + EXPECT_EQ( callbacks.load(), 0 ); + + Http::setThreadPool( nullptr ); + threadPool.reset(); + System::FileSystem::fileRemove( outputPath ); +} + +UTEST( Http, sharedThreadPoolJoinsRunningRequestOnPoolClear ) { + Http::setThreadPool( nullptr ); + Http::Pool::getGlobal().clear(); + + TcpListener listener; + ASSERT_EQ( listener.listen( Socket::AnyPort, IpAddress::LocalHost ), Socket::Done ); + std::thread server( [&listener] { + TcpSocket client; + if ( listener.accept( client ) != Socket::Done ) + return; + char buffer[1024]; + std::size_t received = 0; + client.receive( buffer, sizeof( buffer ), received ); + client.disconnect(); + } ); + + auto threadPool = System::ThreadPool::createShared( 1 ); + Http::setThreadPool( threadPool ); + const URI uri( String::format( "http://127.0.0.1:%u/", listener.getLocalPort() ) ); + auto http = Http::Pool::getGlobal().get( uri ); + ThreadGate progressGate; + Http::Request request( "/" ); + request.setProgressCallback( [&]( const Http&, const Http::Request&, const Http::Response&, + Http::Request::Status status, std::size_t, std::size_t ) { + if ( status == Http::Request::Connected ) + progressGate.enterAndWait(); + return true; + } ); + std::atomic callbacks{ 0 }; + http->sendAsyncRequest( [&]( const Http&, Http::Request&, Http::Response& ) { callbacks++; }, + request, Seconds( 5 ) ); + http.reset(); + ASSERT_TRUE( progressGate.waitUntilEntered() ); + + std::atomic clearCompleted{ false }; + std::thread clearThread( [&] { + Http::Pool::getGlobal().clear(); + clearCompleted.store( true, std::memory_order_release ); + } ); + std::this_thread::sleep_for( std::chrono::milliseconds( 50 ) ); + EXPECT_FALSE( clearCompleted.load( std::memory_order_acquire ) ); + + progressGate.release(); + clearThread.join(); + server.join(); + listener.close(); + EXPECT_TRUE( clearCompleted.load( std::memory_order_acquire ) ); + EXPECT_EQ( callbacks.load(), 0 ); + + Http::setThreadPool( nullptr ); + threadPool.reset(); +} + +UTEST( Http, sharedThreadPoolAllowsPoolClearFromCallback ) { + Http::setThreadPool( nullptr ); + Http::Pool::getGlobal().clear(); + + TcpListener listener; + ASSERT_EQ( listener.listen( Socket::AnyPort, IpAddress::LocalHost ), Socket::Done ); + std::thread server( [&listener] { + TcpSocket client; + if ( listener.accept( client ) != Socket::Done ) + return; + + std::string request; + char buffer[1024]; + std::size_t received = 0; + while ( request.find( "\r\n\r\n" ) == std::string::npos ) { + if ( client.receive( buffer, sizeof( buffer ), received ) != Socket::Done ) + return; + request.append( buffer, received ); + } + const std::string response = "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n\r\nok"; + client.send( response.data(), response.size() ); + client.disconnect(); + } ); + + auto threadPool = System::ThreadPool::createShared( 1 ); + Http::setThreadPool( threadPool ); + const URI uri( String::format( "http://127.0.0.1:%u/", listener.getLocalPort() ) ); + auto http = Http::Pool::getGlobal().get( uri ); + std::mutex callbackMutex; + std::condition_variable callbackCondition; + bool callbackCompleted = false; + bool callbackResponseOk = false; + std::atomic queuedCallbacks{ 0 }; + ThreadGate callbackGate; + http->sendAsyncRequest( + [&]( const Http&, Http::Request&, Http::Response& response ) { + callbackResponseOk = response.getStatus() == Http::Response::Ok; + callbackGate.enterAndWait(); + Http::Pool::getGlobal().clear(); + { + std::lock_guard lock( callbackMutex ); + callbackCompleted = true; + } + callbackCondition.notify_all(); + }, + Http::Request( "/" ), Seconds( 5 ) ); + ASSERT_TRUE( callbackGate.waitUntilEntered() ); + // This operation is queued behind the callback on the same one-thread executor. Pool clearing + // from the callback must not wait for work that only this worker can drain. + http->sendAsyncRequest( + [&]( const Http&, Http::Request&, Http::Response& ) { queuedCallbacks++; }, + Http::Request( "/queued" ), Seconds( 5 ) ); + http.reset(); + callbackGate.release(); + + { + std::unique_lock lock( callbackMutex ); + ASSERT_TRUE( callbackCondition.wait_for( lock, std::chrono::seconds( 5 ), + [&] { return callbackCompleted; } ) ); + } + ThreadGate completionGate; + threadPool->run( [&] { completionGate.enterAndWait(); } ); + ASSERT_TRUE( completionGate.waitUntilEntered() ); + completionGate.release(); + + server.join(); + listener.close(); + EXPECT_TRUE( callbackResponseOk ); + EXPECT_EQ( queuedCallbacks.load(), 0 ); + Http::setThreadPool( nullptr ); + threadPool.reset(); + Http::Pool::getGlobal().clear(); +} + #if EE_PLATFORM != EE_PLATFORM_WIN UTEST( Http, tcpConnectTimeoutHandlesFdAboveFdSetSize ) { TcpListener listener; diff --git a/src/tests/unit_tests/resource_prerequisite_tests.cpp b/src/tests/unit_tests/resource_prerequisite_tests.cpp index 5a9de5fc8..158d1c2a0 100644 --- a/src/tests/unit_tests/resource_prerequisite_tests.cpp +++ b/src/tests/unit_tests/resource_prerequisite_tests.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -89,6 +90,29 @@ class TestTextureAtlasLoader : public TextureAtlasLoader { static_assert( !std::is_copy_constructible::value, "Texture must not be copyable" ); static_assert( !std::is_copy_assignable::value, "Texture must not be copy-assignable" ); +UTEST( ResourcePrerequisites, lruCacheEvictsOnlyLeastRecentlyUsedAndReleasesKeys ) { + LRUCache<2, int, bool> recencyCache; + recencyCache.put( 1, true ); + recencyCache.put( 2, false ); + ASSERT_TRUE( recencyCache.get( 1 ).has_value() ); + recencyCache.put( 3, true ); + + EXPECT_TRUE( !recencyCache.get( 2 ).has_value() ); + EXPECT_TRUE( recencyCache.get( 1 ).has_value() ); + EXPECT_TRUE( recencyCache.get( 3 ).has_value() ); + + auto ownedKey = std::make_shared( 1 ); + std::weak_ptr weakKey = ownedKey; + LRUCache<2, std::shared_ptr, bool> owningKeyCache; + static_assert( decltype( owningKeyCache )::is_static() ); + owningKeyCache.put( ownedKey, true ); + ownedKey.reset(); + ASSERT_TRUE( !weakKey.expired() ); + + owningKeyCache.clear(); + EXPECT_TRUE( weakKey.expired() ); +} + UTEST( ResourcePrerequisites, textureAtlasLoaderAppliesFilterToEveryTexture ) { Engine::instance()->createWindow( WindowSettings( 64, 64, "TextureAtlasLoader filter test", WindowStyle::Default, WindowBackend::Default, diff --git a/src/tests/unit_tests/systemfontresolver_tests.cpp b/src/tests/unit_tests/systemfontresolver_tests.cpp index 58e2e53bc..154afe636 100644 --- a/src/tests/unit_tests/systemfontresolver_tests.cpp +++ b/src/tests/unit_tests/systemfontresolver_tests.cpp @@ -6,6 +6,10 @@ #include #include +#if EE_PLATFORM == EE_PLATFORM_LINUX +#include +#endif + using namespace EE; using namespace EE::Graphics; using namespace EE::System; @@ -14,6 +18,22 @@ static std::string getFontsDir() { return Sys::getProcessPath() + "../assets/fonts/"; } +#if EE_PLATFORM == EE_PLATFORM_LINUX +static std::size_t getOpenFileDescriptorCount() { + DIR* directory = opendir( "/proc/self/fd" ); + if ( !directory ) + return 0; + + std::size_t count = 0; + while ( dirent* entry = readdir( directory ) ) { + if ( entry->d_name[0] != '.' ) + ++count; + } + closedir( directory ); + return count; +} +#endif + UTEST( SystemFontResolver, singletonLifecycle ) { SystemFontResolver::setEnabled( true ); UTEST_PRINT_STEP( "Create singleton" ); @@ -242,6 +262,27 @@ UTEST( SystemFontResolver, getFallbackForCodepoint ) { SystemFontResolver::destroySingleton(); } +#if EE_PLATFORM == EE_PLATFORM_LINUX +UTEST( SystemFontResolver, fallbackProbeDoesNotRetainFontDescriptors ) { + SystemFontResolver::setEnabled( true ); + auto* resolver = SystemFontResolver::instance(); + resolver->enumerate(); + + const std::size_t descriptorsBefore = getOpenFileDescriptorCount(); + resolver->getFallbackForCodepoint( 0x10FFFF, FontWeight::Normal, false ); + const std::size_t descriptorsAfter = getOpenFileDescriptorCount(); + + EXPECT_TRUE_MSG( descriptorsAfter <= descriptorsBefore + 4, + String::format( "Fallback probing changed the descriptor count by %lld", + static_cast( descriptorsAfter ) - + static_cast( descriptorsBefore ) ) + .c_str() ); + + SystemFontResolver::setEnabled( false ); + SystemFontResolver::destroySingleton(); +} +#endif + UTEST( SystemFontResolver, invalidateCache ) { SystemFontResolver::setEnabled( true ); auto* resolver = SystemFontResolver::instance(); diff --git a/src/tests/unit_tests/uiwebview_tests.cpp b/src/tests/unit_tests/uiwebview_tests.cpp index f80e5ed0c..b1f93470f 100644 --- a/src/tests/unit_tests/uiwebview_tests.cpp +++ b/src/tests/unit_tests/uiwebview_tests.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -11,11 +12,14 @@ #include #include #include +#include #include #include #include +#include #include #include +#include #include #include #include @@ -39,7 +43,7 @@ using namespace EE::Window; using namespace EE::Scene; using namespace EE::UI; -static bool readHttpRequestHeaders( TcpSocket& client ) { +static bool readHttpRequestHeaders( TcpSocket& client, std::string* headers = nullptr ) { std::string request; char buffer[1024]; std::size_t received = 0; @@ -49,6 +53,8 @@ static bool readHttpRequestHeaders( TcpSocket& client ) { return false; request.append( buffer, received ); } + if ( headers ) + *headers = std::move( request ); return true; } @@ -3100,3 +3106,171 @@ UTEST( UIWebView, NewerNavigationSupersedesStartedLoad ) { Engine::destroySingleton(); } + +UTEST( UIWebView, RepeatedRemoteNavigationHandlesSubresourceFanOut ) { + constexpr int NavigationCount = 6; + constexpr int ImagesPerDocument = 100; + constexpr int StyleSheetsPerDocument = 4; + constexpr int ResourcesPerDocument = ImagesPerDocument + StyleSheetsPerDocument; + + auto win = Engine::instance()->createWindow( + WindowSettings( 800, 600, "UIWebView Subresource Fan-out Test", WindowStyle::Default, + WindowBackend::Default, 32, {}, 1, false, true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + FileSystem::changeWorkingDirectory( Sys::getProcessPath() ); + + FontTrueType* font = FontTrueType::New( "NotoSans-Regular" ); + font->loadFromFile( "../assets/fonts/NotoSans-Regular.ttf" ); + ASSERT_TRUE( font != nullptr && font->loaded() ); + FontFamily::loadFromRegular( font ); + + std::string imageData; + const std::string imagePath = + Sys::getProcessPath() + "assets/html/reddit_old_thread_files/pixel.png"; + ASSERT_TRUE( FileSystem::fileGet( imagePath, imageData ) ); + ASSERT_FALSE( imageData.empty() ); + + TcpListener listener; + ASSERT_EQ( listener.listen( Socket::AnyPort, IpAddress::LocalHost ), Socket::Done ); + const unsigned short serverPort = listener.getLocalPort(); + std::atomic stopServer{ false }; + std::atomic serverOk{ true }; + std::atomic documentRequests{ 0 }; + std::atomic resourceRequests{ 0 }; + + std::thread server( [&] { + while ( !stopServer.load( std::memory_order_acquire ) ) { + TcpSocket client; + if ( listener.accept( client ) != Socket::Done ) { + if ( !stopServer.load( std::memory_order_acquire ) ) + serverOk.store( false, std::memory_order_release ); + break; + } + + if ( stopServer.load( std::memory_order_acquire ) ) + break; + + std::string requestHeaders; + if ( !readHttpRequestHeaders( client, &requestHeaders ) ) { + serverOk.store( false, std::memory_order_release ); + break; + } + + std::string body; + std::string contentType; + if ( String::startsWith( requestHeaders, "GET /page/" ) ) { + const int document = documentRequests.fetch_add( 1, std::memory_order_acq_rel ); + contentType = "text/html"; + body = ""; + for ( int i = 0; i < StyleSheetsPerDocument; ++i ) + body += ""; + body += "
"; + for ( int i = 0; i < ImagesPerDocument; ++i ) + body += ""; + body += "
"; + } else if ( String::startsWith( requestHeaders, "GET /style.css?" ) ) { + resourceRequests.fetch_add( 1, std::memory_order_acq_rel ); + contentType = "text/css"; + body = "body { color: #223344; }"; + } else if ( String::startsWith( requestHeaders, "GET /pixel.png?" ) ) { + resourceRequests.fetch_add( 1, std::memory_order_acq_rel ); + contentType = "image/png"; + body = imageData; + } else { + serverOk.store( false, std::memory_order_release ); + break; + } + + const std::string response = + "HTTP/1.1 200 OK\r\nContent-Type: " + contentType + + "\r\nContent-Length: " + String::toString( static_cast( body.size() ) ) + + "\r\nConnection: close\r\n\r\n" + body; + if ( client.send( response.data(), response.size() ) != Socket::Done ) { + serverOk.store( false, std::memory_order_release ); + break; + } + client.disconnect(); + } + } ); + + auto httpThreadPool = ThreadPool::createShared( 8 ); + Http::Pool::getGlobal().clear(); + Http::setThreadPool( httpThreadPool ); + + UISceneNode* sceneNode = UISceneNode::New(); + SceneManager::instance()->add( sceneNode ); + sceneNode->getUIThemeManager()->setDefaultFont( font ); + sceneNode->setThreadPool( httpThreadPool ); + + UIWebView* webView = UIWebView::New(); + webView->setParent( sceneNode->getRoot() ); + webView->setPixelsSize( 640, 420 ); + webView->setLayoutSizePolicy( SizePolicy::Fixed, SizePolicy::Fixed ); + + auto pumpUntil = [&]( const std::function& condition, Time timeout ) { + Clock clock; + while ( !condition() && clock.getElapsedTime() < timeout ) { + win->getInput()->update(); + SceneManager::instance()->update( Seconds( 1.f / 60.f ) ); + Sys::sleep( Milliseconds( 1 ) ); + } + return condition(); + }; + + bool allNavigationsStarted = true; + for ( int navigation = 0; navigation < NavigationCount; ++navigation ) { + webView->loadURI( + URI( String::format( "http://127.0.0.1:%u/page/%d", serverPort, navigation ) ) ); + const int minimumResources = navigation * ResourcesPerDocument + 8; + if ( !pumpUntil( + [&] { + return documentRequests.load( std::memory_order_acquire ) >= navigation + 1 && + resourceRequests.load( std::memory_order_acquire ) >= minimumResources; + }, + Seconds( 10 ) ) ) { + allNavigationsStarted = false; + break; + } + } + + const int expectedResourceRequests = NavigationCount * ResourcesPerDocument; + const bool allResourcesLoaded = + allNavigationsStarted && + pumpUntil( + [&] { + return resourceRequests.load( std::memory_order_acquire ) == + expectedResourceRequests; + }, + Seconds( 15 ) ); + + for ( int i = 0; i < 30; ++i ) { + win->getInput()->update(); + SceneManager::instance()->update( Seconds( 1.f / 60.f ) ); + } + + Http::Pool::getGlobal().clear(); + Http::setThreadPool( nullptr ); + httpThreadPool.reset(); + stopServer.store( true, std::memory_order_release ); + TcpSocket wakeServer; + wakeServer.connect( IpAddress::LocalHost, serverPort, Seconds( 1 ) ); + wakeServer.disconnect(); + listener.close(); + if ( server.joinable() ) + server.join(); + + EXPECT_TRUE( serverOk.load( std::memory_order_acquire ) ); + EXPECT_TRUE( allNavigationsStarted ); + EXPECT_TRUE( allResourcesLoaded ); + EXPECT_EQ( NavigationCount, documentRequests.load( std::memory_order_acquire ) ); + EXPECT_EQ( expectedResourceRequests, resourceRequests.load( std::memory_order_acquire ) ); + EXPECT_TRUE( webView->getCurrentURI().toString().find( "/page/5" ) != std::string::npos ); + UISceneNode* documentScene = webView->getDocumentSceneNode(); + ASSERT_TRUE( documentScene != nullptr ); + EXPECT_TRUE( documentScene->getRoot()->find( "document-5" ) != nullptr ); + + Engine::destroySingleton(); +}