diff --git a/src/.DS_Store b/src/.DS_Store new file mode 100644 index 0000000..fe7d9fe Binary files /dev/null and b/src/.DS_Store differ diff --git a/src/api.cpp b/src/api.cpp index d74f457..e8deb82 100644 --- a/src/api.cpp +++ b/src/api.cpp @@ -119,10 +119,25 @@ std::vector> 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); +}