From bb2e3741b282b354953c257d8183aa8c23b55c05 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 26 Sep 2026 15:42:35 -0700 Subject: [PATCH] Serialise the flex scanner: concurrent parses crashed The scanner is non-reentrant global state, and nothing stopped two threads using it at once -- the shared file cache even parsed outside its lock on the understanding that doing so was 'never wrong'. Two threads parsing together lexed each other's input and crashed. In BelfrySCAD that was an intermittent 0xC0000005 on Windows (5 in 600 runs of a GUI test driver): a render worker parsing its file while the UI thread parsed the buffer for the Customizer, both caught inside the parser by faulthandler. parseAst now holds a mutex across lexerBeginString..parse..lexerEnd, and ends the lexer even when parse() throws. A new test parses on 8 threads at once, each checking it got its own tree back: without the lock it crashed every run (SIGSEGV/SIGABRT), with it it passes. Co-Authored-By: Claude Opus 5.5 --- src/.DS_Store | Bin 0 -> 6148 bytes src/api.cpp | 33 ++++++++++++++----- tests/CMakeLists.txt | 1 + tests/test_concurrent_parse.cpp | 56 ++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 9 deletions(-) create mode 100644 src/.DS_Store create mode 100644 tests/test_concurrent_parse.cpp diff --git a/src/.DS_Store b/src/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..fe7d9fe9e39a5609b5106e926b650dada4dc75d3 GIT binary patch literal 6148 zcmeHKu};G<5Iwh*2m+*zOehONOsIbls_+4*KR`=SK}twf%EA;R*!c&3fPoKS><(YR z$_`@dJD+JCLL*oZLU+~qIp;gy`K5|&A~Nmsev7C{L;;kswv6Tr;c?cI)SP7%Xn2lR z=P*l$B|kO%T?Y8w)u~HoG)@No`@7tpkEB@~_mZp!OK|gawg0v`+4V>J6{Evrf7SH3dwcXxTd;4`h?_7WIR58C_^YER@ zK7>%l$YCyMKOJc76#&?P*$VphOa%> parseAst(const std::string& code, const st ParserDriver driver(origin); driver.strictCommas = g_strictCommas; - lexerBeginString(code); - yy::parser parser(driver); - int rc = parser.parse(); - lexerEnd(); + int rc; + { + // The flex scanner is global state (grammar/lexer_api.hpp), so two + // threads parsing at once lexed each other's input and crashed -- in + // BelfrySCAD, a render worker parsing its file while the UI thread + // parsed the buffer for the Customizer: an intermittent 0xC0000005 + // on Windows. One parse at a time; a parse is milliseconds, and the + // shared file cache means a library is parsed once, not per thread. + // ponytail: a global lock; a reentrant scanner (%option reentrant + + // a yyscan_t through the driver) if parsing ever needs to scale. + static std::mutex scannerMutex; + std::lock_guard lock(scannerMutex); + struct EndLexer { // lexerEnd() even when parse() throws + ~EndLexer() { lexerEnd(); } + } endLexer; + lexerBeginString(code); + yy::parser parser(driver); + rc = parser.parse(); + } if (rc != 0 || driver.hadError) { throw ParseError(formatSyntaxError(driver, code, origin, sourceMap)); @@ -409,11 +424,11 @@ FileAstPtr parseFileShared(const std::string& absPath, bool includeComments) { return it->second.ast; } - // Parsed OUTSIDE the lock: parsing a library takes tens of - // milliseconds, and holding a global lock across it would serialise - // every thread. Two threads racing the same file both parse and one - // result is dropped -- wasteful once, never wrong, and far cheaper than - // the alternative. + // Parsed outside THIS lock, so a cache lookup never waits behind a + // parse. (parseAst serialises the scanner itself: this comment once + // called concurrent parses "never wrong", and they were -- the scanner + // is global state.) Two threads racing the same file each parse it in + // turn and one result is dropped: wasteful once, never wrong. auto parsed = std::make_shared(parseSingleFile(absPath, includeComments)); std::lock_guard lock(g_astCacheMutex); CacheEntry& entry = g_astCache[key]; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d8f9fe6..5dc90a1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -27,6 +27,7 @@ add_executable(oscad_tests test_node_str.cpp test_render_expression.cpp test_strict_commas.cpp + test_concurrent_parse.cpp ) target_link_libraries(oscad_tests PRIVATE openscad_cpp_parser GTest::gtest_main) diff --git a/tests/test_concurrent_parse.cpp b/tests/test_concurrent_parse.cpp new file mode 100644 index 0000000..c5d2f0b --- /dev/null +++ b/tests/test_concurrent_parse.cpp @@ -0,0 +1,56 @@ +// The flex scanner is non-reentrant, global state (grammar/lexer_api.hpp), +// so two threads parsing at once used to lex each other's input. In +// BelfrySCAD that was an intermittent 0xC0000005 on Windows: a render worker +// parsing its file while the UI thread parsed the buffer for the Customizer. +// parseAst now serialises the scanner. Every thread here parses its OWN +// source and checks it got its own tree back, so corruption fails the test +// even where it does not crash. +#include "openscad_cpp_parser/api.hpp" + +#include + +#include +#include +#include +#include + +using namespace oscad; + +namespace { +std::string sourceFor(int thread, int round) { + // Different identifiers and lengths per thread, so a lexer reading + // another thread's buffer produces a different tree, not an equal one. + std::string name = "v" + std::to_string(thread) + "_" + std::to_string(round); + std::string src; + for (int i = 0; i < 20 + thread; ++i) { + src += name + "_" + std::to_string(i) + " = [" + std::to_string(i) + ", " + + std::to_string(thread) + ", \"s" + std::to_string(round) + "\"];\n"; + } + return src; +} +} // namespace + +TEST(ConcurrentParse, ThreadsParsingAtOnceEachGetTheirOwnTree) { + constexpr int kThreads = 8, kRounds = 150; + std::atomic wrong{0}; + std::vector threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([t, &wrong] { + for (int r = 0; r < kRounds; ++r) { + const std::string src = sourceFor(t, r); + try { + auto ast = parseAst(src); + const auto* first = ast.empty() ? nullptr : dynamic_cast(ast[0].get()); + const std::string want = "v" + std::to_string(t) + "_" + std::to_string(r) + "_0"; + if (ast.size() != static_cast(20 + t) || !first || first->name->name != want) { + ++wrong; + } + } catch (...) { + ++wrong; // a syntax error from lexing another thread's input + } + } + }); + } + for (auto& th : threads) th.join(); + EXPECT_EQ(wrong.load(), 0); +}