From 51b88fa12501c8a7f199bb2625e9d34ceb3c3f09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Lucas=20Golini?= Date: Mon, 21 Sep 2026 01:55:58 -0300 Subject: [PATCH] fix(ui): harden asynchronous document search Snapshot document search state before dispatching highlight workers and retain the searched document for the duration of each job. Publish results through MainThreadLifetime and reject results when the editor has changed document or search parameters. Remove the obsolete worker counter and destructor wait used for editor lifetime management. Use atomic cancellation flags in TextDocument::findAll() and protect active-search map inspection during destruction. Add regression coverage for concurrent cancellation, stale highlight results, rapid backtick searches after a large multiline paste, and editor destruction during active search. Refs SpartanJ/ecode#956. --- include/eepp/ui/doc/textdocument.hpp | 2 +- include/eepp/ui/uicodeeditor.hpp | 1 - src/eepp/ui/doc/textdocument.cpp | 17 ++- src/eepp/ui/uicodeeditor.cpp | 63 ++++++---- src/tests/unit_tests/textdocument_tests.cpp | 34 ++++++ src/tests/unit_tests/uicodeeditor_tests.cpp | 124 +++++++++++++++++++- 6 files changed, 209 insertions(+), 32 deletions(-) diff --git a/include/eepp/ui/doc/textdocument.hpp b/include/eepp/ui/doc/textdocument.hpp index 86b30a6cc..ec0f285c5 100644 --- a/include/eepp/ui/doc/textdocument.hpp +++ b/include/eepp/ui/doc/textdocument.hpp @@ -833,7 +833,7 @@ class EE_API TextDocument { size_t mLastSelection{ 0 }; std::unique_ptr mHighlighter; Mutex mStopFlagsMutex; - UnorderedMap> mStopFlags; + UnorderedMap> mStopFlags; FoldRangeService mFoldRangeService; void initializeCommands(); diff --git a/include/eepp/ui/uicodeeditor.hpp b/include/eepp/ui/uicodeeditor.hpp index 9cbb93ae4..46ec1c000 100644 --- a/include/eepp/ui/uicodeeditor.hpp +++ b/include/eepp/ui/uicodeeditor.hpp @@ -960,7 +960,6 @@ class EE_API UICodeEditor : public UITouchDraggableWidget, public TextDocument:: Time mFoldsRefreshTime; Uint32 mTabWidth; Uint32 mLigatureFeatures{ 0 }; - std::atomic mHighlightWordProcessing{ false }; TextRange mLinkPosition; String mLink; Vector2f mScroll; diff --git a/src/eepp/ui/doc/textdocument.cpp b/src/eepp/ui/doc/textdocument.cpp index 8beafcfed..8b37bad97 100644 --- a/src/eepp/ui/doc/textdocument.cpp +++ b/src/eepp/ui/doc/textdocument.cpp @@ -338,8 +338,14 @@ TextDocument::~TextDocument() { mHighlighter->setStopTokenizingAsync(); // TODO: Use a condition variable to wait the thread pool to finish - while ( !mStopFlags.empty() ) + while ( true ) { + { + Lock l( mStopFlagsMutex ); + if ( mStopFlags.empty() ) + break; + } Sys::sleep( Milliseconds( 0.1 ) ); + } if ( mLoading ) { mLoading = false; @@ -3722,7 +3728,7 @@ TextDocument::SearchResult TextDocument::findLast( const String& text, TextPosit void TextDocument::stopActiveFindAll() { Lock l( mStopFlagsMutex ); for ( const auto& stopFlag : mStopFlags ) - *stopFlag.second.get() = true; + stopFlag.second->store( true, std::memory_order_relaxed ); } bool TextDocument::isDoingTextInput() const { @@ -3739,8 +3745,8 @@ TextDocument::SearchResults TextDocument::findAll( const String& text, bool case SearchResults all; TextDocument::SearchResult found; TextPosition from = startOfDoc(); - auto stopFlagUP = std::make_unique( false ); - bool* stopFlag = stopFlagUP.get(); + auto stopFlagUP = std::make_unique( false ); + std::atomic_bool* stopFlag = stopFlagUP.get(); { Lock l( mStopFlagsMutex ); mStopFlags.insert( { stopFlag, std::move( stopFlagUP ) } ); @@ -3755,7 +3761,8 @@ TextDocument::SearchResults TextDocument::findAll( const String& text, bool case break; from = found.result.end(); all.push_back( found ); - if ( ( maxResults != 0 && all.size() >= maxResults ) || *stopFlag ) + if ( ( maxResults != 0 && all.size() >= maxResults ) || + stopFlag->load( std::memory_order_relaxed ) ) break; } } while ( found.isValid() ); diff --git a/src/eepp/ui/uicodeeditor.cpp b/src/eepp/ui/uicodeeditor.cpp index 84db768c1..274c52a1e 100644 --- a/src/eepp/ui/uicodeeditor.cpp +++ b/src/eepp/ui/uicodeeditor.cpp @@ -245,11 +245,6 @@ UICodeEditor::~UICodeEditor() { // Remember to stop all the async find jobs mDoc->stopActiveFindAll(); - // TODO: Use a condition variable to wait the thread pool to finish - // Wait to end all the async find jobs - while ( mHighlightWordProcessing ) - Sys::sleep( Milliseconds( 0.1 ) ); - mDocView.setDocument( nullptr ); std::size_t clientsOfTypeCount = mDoc->clientOfTypeCount( TextDocument::Client::Type::Core ); long useCount = mDoc.use_count(); @@ -3968,6 +3963,16 @@ const TextSearchParams& UICodeEditor::getHighlightWord() const { } void UICodeEditor::updateHighlightWordCache() { + struct HighlightSearchJob { + HighlightSearchJob( std::shared_ptr searchedDocument, + TextSearchParams searchedParams ) : + document( std::move( searchedDocument ) ), params( std::move( searchedParams ) ) {} + + const std::shared_ptr document; + const TextSearchParams params; + TextRanges ranges; + }; + if ( mHighlightWord.isEmpty() ) return; @@ -3976,33 +3981,43 @@ void UICodeEditor::updateHighlightWordCache() { removeActionsByTag( tag ); runOnMainThread( [this, tag]() { - getUISceneNode()->getThreadPool()->removeWithTag( tag ); - getUISceneNode()->getThreadPool()->run( - [this]() { - if ( mDoc->isRunningTransaction() ) + auto threadPool = getUISceneNode()->getThreadPool(); + threadPool->removeWithTag( tag ); + auto search = std::make_shared( mDoc, mHighlightWord ); + const auto lifetime = mAsyncLifetime.weakHandle(); + threadPool->run( + [search, lifetime]() { + if ( search->document->isRunningTransaction() ) return; Clock docSearch; - mHighlightWordProcessing++; - mDoc->stopActiveFindAll(); + search->document->stopActiveFindAll(); - auto wordCache = mDoc->findAll( - mHighlightWord.escapeSequences ? String::unescape( mHighlightWord.text ) - : mHighlightWord.text, - mHighlightWord.caseSensitive, mHighlightWord.wholeWord, - mHighlightWord.type, mHighlightWord.range ); - - { - Lock l( mHighlightWordCacheMutex ); - mHighlightWordCache = wordCache.ranges(); - } + const String searchedText = search->params.escapeSequences + ? String::unescape( search->params.text ) + : search->params.text; + search->ranges = search->document + ->findAll( searchedText, search->params.caseSensitive, + search->params.wholeWord, + search->params.type, search->params.range ) + .ranges(); Log::info( "Document search triggered in document: \"%s\", searched for " "\"%s\" and took %.2f ms", - mDoc->getFilename().c_str(), - mHighlightWord.text.toUtf8().c_str(), + search->document->getFilename(), search->params.text.toUtf8(), docSearch.getElapsedTime().asMilliseconds() ); + + lifetime.run( [search]( UICodeEditor* editor ) { + if ( editor->mDoc != search->document || + editor->mHighlightWord != search->params ) + return; + { + Lock l( editor->mHighlightWordCacheMutex ); + editor->mHighlightWordCache = std::move( search->ranges ); + } + editor->invalidateDraw(); + } ); }, - [this]( const auto& ) { mHighlightWordProcessing--; }, tag ); + {}, tag ); }, Milliseconds( 16 ), tag ); } else { diff --git a/src/tests/unit_tests/textdocument_tests.cpp b/src/tests/unit_tests/textdocument_tests.cpp index a3da9537c..b51a6d2f1 100644 --- a/src/tests/unit_tests/textdocument_tests.cpp +++ b/src/tests/unit_tests/textdocument_tests.cpp @@ -1,8 +1,10 @@ #include "utest.hpp" #include +#include #include #include #include +#include using namespace EE::UI::Doc; using namespace EE::System; @@ -171,6 +173,38 @@ UTEST( TextDocument, insertLargeMultilineBlock ) { EXPECT_STRINGEQ( "tail\n", doc.line( insertedLineCount + 2 ).getText() ); } +UTEST( TextDocument, findAllCanBeCancelledConcurrently ) { + constexpr size_t lineCount = 65536; + String text; + text.reserve( lineCount * 48 ); + for ( size_t i = 0; i < lineCount; ++i ) + text.append( "needle xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" ); + + TextDocument doc; + doc.textInput( text ); + TextDocument::SearchResults results; + std::atomic_bool started{ false }; + std::atomic_bool finished{ false }; + std::thread worker( [&] { + started.store( true, std::memory_order_release ); + results = doc.findAll( "needle" ); + finished.store( true, std::memory_order_release ); + } ); + + while ( !started.load( std::memory_order_acquire ) ) + Sys::sleep( Milliseconds( 0.1 ) ); + while ( !finished.load( std::memory_order_acquire ) ) { + doc.stopActiveFindAll(); + Sys::sleep( Milliseconds( 0.1 ) ); + } + worker.join(); + + EXPECT_TRUE( results.size() <= lineCount ); + EXPECT_TRUE( results.empty() || results.isSorted() ); + for ( const auto& result : results ) + EXPECT_TRUE( result.isValid() ); +} + UTEST( TextDocument, multilineInsertCursorEndsBeforeExistingSuffix ) { TextDocument doc; doc.insert( 0, { 0, 0 }, "prefix-suffix" ); diff --git a/src/tests/unit_tests/uicodeeditor_tests.cpp b/src/tests/unit_tests/uicodeeditor_tests.cpp index 7c25f6a54..1a9483e8c 100644 --- a/src/tests/unit_tests/uicodeeditor_tests.cpp +++ b/src/tests/unit_tests/uicodeeditor_tests.cpp @@ -1,4 +1,5 @@ -#include "utest.h" +#include "utest.hpp" +#include #include #include #include @@ -23,8 +24,48 @@ class TestableCodeEditor : public UICodeEditor { bool isLongestLineWidthDirtyForTest() const { return mLongestLineWidthDirty; } void clearLongestLineWidthDirtyForTest() { mLongestLineWidthDirty = false; } + + TextRanges getHighlightWordCacheForTest() { + Lock l( mHighlightWordCacheMutex ); + return mHighlightWordCache; + } + + void clearHighlightWordCacheForTest() { + Lock l( mHighlightWordCacheMutex ); + mHighlightWordCache.clear(); + } }; +template +static bool waitForCondition( Predicate&& predicate, const Time& timeout = Seconds( 10 ) ) { + Clock clock; + while ( !predicate() && clock.getElapsedTime() < timeout ) + Sys::sleep( Milliseconds( 1 ) ); + return predicate(); +} + +static bool waitForThreadPool( const std::shared_ptr& threadPool ) { + auto completed = std::make_shared( false ); + threadPool->run( [completed] { completed->store( true, std::memory_order_release ); } ); + return waitForCondition( [completed] { return completed->load( std::memory_order_acquire ); } ); +} + +static void dispatchDebouncedEditorWork() { + Sys::sleep( Milliseconds( 20 ) ); + SceneManager::instance()->update(); +} + +static String makeLargeMarkdownDocument() { + constexpr size_t blockCount = 16384; + String text; + text.reserve( blockCount * 80 ); + for ( size_t i = 0; i < blockCount; ++i ) { + text.append( "## Heading\nInline `code` and ``span``.\n```cpp\nvalue\n```\n" ); + } + text.append( "unique-final-token\n" ); + return text; +} + UTEST( SyntaxColorScheme, CopiesShareStorageAndDetachOnMutation ) { auto defaults = SyntaxColorScheme::getDefaultDark(); auto copy = defaults; @@ -133,6 +174,87 @@ UTEST( UICodeEditor, DefersLongestLineMeasurementForLargeChanges ) { eeDelete( editor ); } +UTEST( UICodeEditor, AsyncHighlightRejectsStaleResultsAfterLargePaste ) { + UIApplication app( WindowSettings{ 320, 240, "eepp - async highlight test" } ); + auto threadPool = ThreadPool::createShared( 1 ); + app.getUI()->setThreadPool( threadPool ); + auto* editor = eeNew( TestableCodeEditor, () ); + editor->setParent( app.getUI()->getRoot() ); + editor->getDocument().textInput( makeLargeMarkdownDocument() ); + + editor->clearHighlightWordCacheForTest(); + editor->setHighlightWord( { "`" } ); + dispatchDebouncedEditorWork(); + ASSERT_TRUE( waitForThreadPool( threadPool ) ); + + // The old result is already queued for the main thread. Changing the query before pumping that + // callback must make the result stale and leave the cache untouched. + editor->setHighlightWord( { "unique-final-token" } ); + SceneManager::instance()->update(); + EXPECT_TRUE( editor->getHighlightWordCacheForTest().empty() ); + + dispatchDebouncedEditorWork(); + ASSERT_TRUE( waitForThreadPool( threadPool ) ); + SceneManager::instance()->update(); + auto ranges = editor->getHighlightWordCacheForTest(); + ASSERT_EQ( size_t{ 1 }, ranges.size() ); + + // Reproduce the #956 workload: a fresh multiline paste followed by rapid literal backtick + // queries while an earlier highlight scan can still be publishing its result. + for ( size_t i = 0; i < 4; ++i ) { + editor->setHighlightWord( { "`" } ); + dispatchDebouncedEditorWork(); + editor->setHighlightWord( { "``" } ); + editor->setHighlightWord( { "```" } ); + dispatchDebouncedEditorWork(); + ASSERT_TRUE( waitForThreadPool( threadPool ) ); + SceneManager::instance()->update(); + } + + EXPECT_STDSTREQ( "```", editor->getHighlightWord().text.toUtf8() ); + auto expected = editor->getDocument().findAll( "```" ).ranges(); + EXPECT_TRUE( expected == editor->getHighlightWordCacheForTest() ); + + eeDelete( editor ); + app.getUI()->setThreadPool( nullptr ); + threadPool.reset(); +} + +UTEST( UICodeEditor, AsyncHighlightSurvivesEditorDestruction ) { + UIApplication app( WindowSettings{ 320, 240, "eepp - async highlight destruction test" } ); + auto threadPool = ThreadPool::createShared( 1 ); + app.getUI()->setThreadPool( threadPool ); + std::atomic_bool blockerStarted{ false }; + std::atomic_bool releaseBlocker{ false }; + threadPool->run( [&] { + blockerStarted.store( true, std::memory_order_release ); + while ( !releaseBlocker.load( std::memory_order_acquire ) ) + Sys::sleep( Milliseconds( 1 ) ); + } ); + const bool started = + waitForCondition( [&] { return blockerStarted.load( std::memory_order_acquire ); } ); + if ( !started ) + releaseBlocker.store( true, std::memory_order_release ); + ASSERT_TRUE( started ); + + auto* editor = eeNew( TestableCodeEditor, () ); + editor->setParent( app.getUI()->getRoot() ); + editor->getDocument().textInput( makeLargeMarkdownDocument() ); + + editor->setHighlightWord( { "missing-highlight-value" } ); + dispatchDebouncedEditorWork(); + const Uint64 tag = reinterpret_cast( editor ); + const bool searchQueued = threadPool->existsTagInQueue( tag ); + releaseBlocker.store( true, std::memory_order_release ); + ASSERT_TRUE( searchQueued ); + ASSERT_TRUE( waitForCondition( [&] { return !threadPool->existsTagInQueue( tag ); } ) ); + eeDelete( editor ); + + app.getUI()->setThreadPool( nullptr ); + threadPool.reset(); + SceneManager::instance()->update(); +} + UTEST( UICodeEditor, DefaultKeybindingCacheTracksConfiguredModifiers ) { const Uint32 originalDefaultModifier = KeyMod::getDefaultModifier(); const Uint32 originalSecondaryModifier = KeyMod::getDefaultSecondaryModifier();