Serialise the flex scanner: concurrent parses crashed

src/api.cpp

@@ -119,10 +119,25 @@ std::vector<std::unique_ptr<ASTNode>> 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<std::mutex> 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<const FileAst>(parseSingleFile(absPath, includeComments));
     std::lock_guard<std::mutex> lock(g_astCacheMutex);
     CacheEntry& entry = g_astCache[key];

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)
 

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 <gtest/gtest.h>
+
+#include <atomic>
+#include <string>
+#include <thread>
+#include <vector>
+
+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<int> wrong{0};
+    std::vector<std::thread> 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<const Assignment*>(ast[0].get());
+                    const std::string want = "v" + std::to_string(t) + "_" + std::to_string(r) + "_0";
+                    if (ast.size() != static_cast<size_t>(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);
+}