-
Notifications
You must be signed in to change notification settings - Fork 34
Fix vm.create child VMs missing native libs and make spawned runtimes libuv-safe
#692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: primary
Are you sure you want to change the base?
Conversation
|
cc @vrn-sn this isn't obsoleted by the changes you made, right? |
Partially, this part is solved by my previous PR:
The rest of the logic in this PR that relates to libuv is unrelated to what I merged in. |
|
Left some preliminary feedback - I think with Varuns PR we can make this one considerably smaller.
I'm not sure there is undefined behaviour - the default loop is single threaded right? So each coroutine only runs on the main thread, and continuations are only processed in the main thread when |
| #include <vector> | ||
|
|
||
| struct lua_State; | ||
| struct uv_loop_s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal libuv type - you'll want to store uv_loop_t
| ~Runtime(); | ||
|
|
||
| bool useDedicatedUvLoop(); | ||
| uv_loop_s* getUvLoop() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| uv_loop_s* getUvLoop() const; | |
| uv_loop_t* getUVLoop() const; |
|
|
||
| std::atomic<int> activeTokens; | ||
|
|
||
| bool ownsUvLoop = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do one runtime, one loop - see my comment below, since we might not actually need this?
| // Event loop for this runtime; defaults to `uv_default_loop()`, but can be dedicated via `useDedicatedUvLoop`. | ||
| uv_loop_s* uvLoop = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a lot of complexity. Instead of defaulting, just initialize a uv_loop manually for this runtime.
| if (ownsUvLoop) | ||
| return true; | ||
|
|
||
| uv_loop_t* loop = new uv_loop_t(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can move into the RuntimeConstructor, along with the uv_loop_init call.
| static std::mutex spawnedRuntimeMutex; | ||
| static std::vector<std::weak_ptr<Runtime>> spawnedRuntimes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shouldn't be static data( I don't think we need a mutex here either?) Each runtime is going to be on it's own event loop.
| { | ||
| uv_fs_t unlink_req; | ||
| int err = uv_fs_unlink(uv_default_loop(), &unlink_req, luaL_checkstring(L, 1), nullptr); | ||
| uv_loop_t* loop = reinterpret_cast<uv_loop_t*>(getRuntime(L)->getUvLoop()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't want to re-interpret cast here - this should just be a uv_loop_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ditto for all the places below.
Summary
This PR fixes a regression introduced in
0.1.0-nightly.20251020wherevm.create()child VMs did not have native@lute/*modules registered (e.g.@lute/net). As a result, child VMs would fall back todefinitions/*.luauand error with"not implemented"(repro:examples/parallel_serve.luau). Closes #691.Additionally, once child VMs can load native libraries again, multiple runtimes running on different OS threads must not share and concurrently drive
uv_default_loop(). This PR gives each spawned runtime its own libuv loop and wires the native libraries to use the runtime’s loop.User-visible fixes
examples/parallel_serve.luauworks again (no longer hitsdefinitions/net.luau: not implementedwhen running@lute/netinside avm.create()VM).Key changes
lute/vm/src/spawn.cpp@lute/*modules in child VMs (matching the main CLI runtime behavior).lute/runtime/include/lute/runtime.h,lute/runtime/src/runtime.cppRuntimenow owns an optional dedicateduv_loop_tand exposesgetUvLoop().registerSpawnedRuntime,waitForSpawnedRuntimes) so the CLI can stay alive while spawned runtimes still have work.lute/cli/src/climain.cppuv_default_loop()activity).libuv-backed libraries now use the calling runtime’s loop instead of
uv_default_loop():lute/net/src/net.cpp(also minimal locking around global server maps for multi-VM access)lute/task/src/task.cpplute/fs/src/fs.cpp,lute/fs/src/fs_impl.cpplute/process/src/process.cpplute/io/src/io.cppTests
vm.create()module loading:tests/src/vmcreate.test.cpptests/src/vm/vm_requirer.luautests/src/vm/vm_helper.luauRepro / Verification
Repro (previously failed on
20251020+):lute tools/luthier.luau run lute -- ./examples/parallel_serve.luauRun tests:
lute tools/luthier.luau run Lute.TestNotes
This avoids undefined behavior from multiple runtime threads concurrently creating handles on and calling
uv_run()on the single process-globaluv_default_loop()(and the associated uWebSockets loop).This was worked on by Codex for an hour, neat. I need to spend some time reviewing everything before opening this from draft