Skip to content

Fix #655: Async issues with threads outliving the chaiscript engine#656

Merged
lefticus merged 2 commits into
ChaiScript:developfrom
leftibot:fix/issue-655-async-issues-with-threads-outliving-the-
Apr 11, 2026
Merged

Fix #655: Async issues with threads outliving the chaiscript engine#656
lefticus merged 2 commits into
ChaiScript:developfrom
leftibot:fix/issue-655-async-issues-with-threads-outliving-the-

Conversation

@leftibot
Copy link
Copy Markdown
Contributor

@leftibot leftibot commented Apr 11, 2026

Automated fix by @leftibot.

What changed

Fix #655: Join async threads before engine destruction to prevent heap-use-after-free
Issues #632 and #636 (PRs #651 and #653) both stem from the same root cause: async
threads spawned via async() can outlive the Dispatch_Engine, accessing shared state
(global objects map, type maps) after it has been destroyed. The fix moves async()
registration from the stdlib module into ChaiScript_Basic, where spawned threads are
tracked via Dispatch_Engine. The engine's destructor now joins all outstanding async
threads before destroying shared data structures.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Files

 CMakeLists.txt                                    |  3 ++
 include/chaiscript/chaiscript_stdlib.hpp          |  5 +--
 include/chaiscript/dispatchkit/dispatchkit.hpp    | 41 ++++++++++++++++++++
 include/chaiscript/language/chaiscript_engine.hpp | 20 ++++++++++
 unittests/async_engine_lifetime.chai              | 17 +++++++++
 unittests/async_engine_lifetime_test.cpp          | 46 +++++++++++++++++++++++
 6 files changed, 129 insertions(+), 3 deletions(-)

Closes #655

Triggered by @lefticus.

@leftibot @claude
Fix ChaiScript#655: Join async threads before engine destruction to p…
8dbb5ed 
…revent heap-use-after-free

Issues ChaiScript#632 and ChaiScript#636 (PRs ChaiScript#651 and ChaiScript#653) both stem from the same root cause: async
threads spawned via async() can outlive the Dispatch_Engine, accessing shared state
(global objects map, type maps) after it has been destroyed. The fix moves async()
registration from the stdlib module into ChaiScript_Basic, where spawned threads are
tracked via Dispatch_Engine. The engine's destructor now joins all outstanding async
threads before destroying shared data structures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lefticus
Copy link
Copy Markdown
Member

lefticus commented Apr 11, 2026

@leftibot prior to this change, ChaiScript engine was copyable, but now it's not?

@leftibot
Copy link
Copy Markdown
Contributor Author

leftibot commented Apr 11, 2026

Good question. The ChaiScript_Basic class (and therefore ChaiScript) was already non-copyable prior to this change because it holds a std::unique_ptr<parser::ChaiScript_Parser_Base> m_parser member (include/chaiscript/language/chaiscript_engine.hpp:75), which deletes the copy constructor implicitly.

What this PR adds is the explicit = delete on Dispatch_Engine's copy constructor and copy assignment (include/chaiscript/dispatchkit/dispatchkit.hpp), which makes the intent clear and provides better error messages, but doesn't change the user-facing API — ChaiScript was already move-only in practice.

That said, the explicit delete is still the right thing to do here since Dispatch_Engine now owns std::vector<std::thread>, which is itself non-copyable — so the compiler would have deleted the copy operations anyway. The explicit annotation just documents the design intent.

@lefticus
Copy link
Copy Markdown
Member

lefticus commented Apr 11, 2026

@leftibot update the code to follow the rule of 5 and explicitly =default the move operations then too

@leftibot @claude
Address review: follow rule of 5, explicitly default move operations
530b492 
Requested by @lefticus in PR ChaiScript#656 review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leftibot
Copy link
Copy Markdown
Contributor Author

leftibot commented Apr 11, 2026

Addressed review feedback from @lefticus. New commits:

530b492b Address review: follow rule of 5, explicitly default move operations

@lefticus lefticus merged commit 7b95ff5 into ChaiScript:develop Apr 11, 2026
5 of 7 checks passed
This was referenced Apr 11, 2026
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lefticus lefticus lefticus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Async issues with threads outliving the chaiscript engine

2 participants

@leftibot @lefticus