Threadopt #1
Threadopt #1
Conversation
…k_error_ext_yieldable
|
Warning Rate limit exceeded@cheesycod has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (23)
WalkthroughThis update introduces explicit auxiliary thread management for Lua stack operations, adds support for continuations and yielding in Lua functions, and refactors stack manipulation traits and methods to require explicit Lua state pointers. It also adds new public types and methods for continuation status, improves error handling, and expands test coverage for threads and continuations. Changes
Sequence Diagram(s)sequenceDiagram
participant Rust as Rust Code
participant Lua as Lua State
participant Aux as Auxiliary Thread
Rust->>Lua: Create auxiliary thread(s) for reference management
Rust->>Lua: Push value onto stack (specifying Aux thread)
Lua->>Aux: Store value in auxiliary thread stack
Rust->>Lua: Create function with continuation
Lua->>Rust: Calls Rust callback (with continuation)
Rust->>Lua: Yield with arguments (using lua_yieldc)
Lua->>Rust: Resume continuation (ContinuationStatus::Yielded/Ok/Error)
Rust->>Lua: Pop or push values directly on specified thread stack
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/multi.rs (1)
15-17:⚠️ Potential issueImport statement is missing for
ffi.The code uses
ffi::lua_Statethroughout but I don't see theffimodule imported. This should cause a compilation error.Add the missing import at the top of the file:
+use crate::ffi; use crate::error::Result;
🧹 Nitpick comments (9)
src/prelude.rs (1)
6-14: Minor ordering nit – keep re-exports alphabetised
ContinuationStatus as LuaContinuationStatusis a welcome addition, but it breaks the alphabetical ordering of the long re-export list. Keeping the list sorted makes future diffs easier to scan.- Chunk as LuaChunk, ContinuationStatus as LuaContinuationStatus, Either as LuaEither, Error as LuaError, + Chunk as LuaChunk, Either as LuaEither, Error as LuaError, ContinuationStatus as LuaContinuationStatus,tests/byte_string.rs (1)
4-9:create_luatest does not assert behaviourThe new
create_luatest only constructs objects and prints them. Without assertions it can’t fail, so it adds no real coverage and slows CI. Consider converting it to a doc-example or adding at least one meaningful assertion (e.g. table length == 0).src/luau/require.rs (1)
449-454: Add a sanity-check to ensure we are pushing onto the correct Lua stack
rawlua.push_at(state, loader)?assumes thatstateis the samelua_State*thatrawluainternally wraps.
A defensive assertion guards against accidentally mixing main/auxiliary threads, which would corrupt the stack at runtime.let loader = this.loader(rawlua.lua())?; - rawlua.push_at(state, loader)?; + debug_assert_eq!( + state, + rawlua.state(), + "Attempting to push a loader onto a mismatched Lua state" + ); + rawlua.push_at(state, loader)?;src/luau/mod.rs (1)
88-94: Optional guard when specifying the target stackAnalogous to
require.rs, it may be worth asserting that thestatepassed to
push_into_specified_stackmatchesrawlua.state()before the push.
This costs nothing in release builds (debug_assert!) and can save hours of
debugging mysterious stack corruption.tests/thread.rs (2)
271-519: Excellent comprehensive continuation test coverage!The test thoroughly covers:
- Basic continuation functionality
- Empty yield arguments
- Recursive/multiple continuations
- Panic propagation in continuations
- ContinuationStatus validation
Consider removing or converting the
println!statements to debug assertions for cleaner test output:- println!("Reached cont"); + // Continuation reached successfully
521-748: Stress test validates thread management, but has some concerns.Good:
- Validates thread identity uniqueness across auxiliary threads
- Ensures debug formatting consistency
- Tests continuation functionality with many threads
Concerns:
- Creating 2 million threads might be excessive for CI environments
- The 100GB memory limit seems unnecessarily high
- The continuation test code (lines 563-747) is duplicated from the previous test
Consider:
- Reducing thread count to a more reasonable number (e.g., 10,000)
- Extracting the duplicated continuation test code into a helper function
- for i in 1..2000000 { + for i in 1..10000 {src/state/extra.rs (1)
95-98: Consider documenting the auxiliary thread growth strategy.The change from a single auxiliary thread to a vector of threads is well-implemented. However, it would be helpful to document:
- When new threads are added to the
ref_threadvector- The maximum number of auxiliary threads allowed
- The purpose of separating
ref_thread_internalfrom the main vectorAlso applies to: 189-190
src/state/raw.rs (2)
15-17: Remove unused import.The
callback_error_extimport is marked with#[allow(unused_imports)]but it appears to be genuinely unused in this file. Onlycallback_error_ext_yieldableis used.-#[allow(unused_imports)] -use crate::state::util::callback_error_ext; -use crate::state::util::{callback_error_ext_yieldable, get_next_spot}; +use crate::state::util::{callback_error_ext_yieldable, get_next_spot};
1284-1400: Consider reducing code duplication between Luau and non-Luau implementations.Both implementations share significant common code in the callback handlers. The main difference is that Luau uses
ffi::lua_pushcclosurecwhile others useffi::lua_pushcclosure.Consider extracting the common callback creation logic:
unsafe fn create_continuation_callback_handlers() -> ( unsafe extern "C-unwind" fn(*mut ffi::lua_State) -> c_int, unsafe extern "C-unwind" fn(*mut ffi::lua_State, c_int) -> c_int, ) { // Return the common callback implementations }Otherwise, the continuation support implementation looks correct and properly handles different Lua versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
mlua-sys/src/lua52/lua.rs(1 hunks)mlua-sys/src/lua53/lua.rs(1 hunks)mlua-sys/src/lua54/lua.rs(1 hunks)mlua-sys/src/luau/lua.rs(1 hunks)src/buffer.rs(1 hunks)src/conversion.rs(34 hunks)src/error.rs(1 hunks)src/function.rs(11 hunks)src/lib.rs(1 hunks)src/luau/mod.rs(1 hunks)src/luau/require.rs(1 hunks)src/multi.rs(9 hunks)src/prelude.rs(1 hunks)src/scope.rs(4 hunks)src/serde/mod.rs(2 hunks)src/state.rs(17 hunks)src/state/extra.rs(6 hunks)src/state/raw.rs(28 hunks)src/state/util.rs(5 hunks)src/string.rs(1 hunks)src/table.rs(25 hunks)src/thread.rs(8 hunks)src/traits.rs(5 hunks)src/types.rs(2 hunks)src/types/value_ref.rs(3 hunks)src/userdata.rs(14 hunks)src/util/types.rs(2 hunks)src/value.rs(2 hunks)tests/byte_string.rs(1 hunks)tests/tests.rs(1 hunks)tests/thread.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/value.rs (2)
src/state/raw.rs (3)
lua(103-105)lua(1515-1515)state(116-118)src/thread.rs (1)
state(114-116)
src/string.rs (1)
src/state/raw.rs (1)
ref_thread(128-136)
src/luau/require.rs (2)
src/state/raw.rs (1)
state(116-118)src/thread.rs (1)
state(114-116)
src/scope.rs (2)
src/state/raw.rs (1)
ref_thread(128-136)src/util/mod.rs (1)
get_metatable_ptr(296-308)
src/util/types.rs (2)
src/state/extra.rs (1)
type_key(157-159)src/util/error.rs (1)
type_key(26-28)
mlua-sys/src/lua53/lua.rs (2)
mlua-sys/src/lua52/lua.rs (1)
lua_yieldc(276-278)mlua-sys/src/lua54/lua.rs (1)
lua_yieldc(303-305)
mlua-sys/src/lua54/lua.rs (2)
mlua-sys/src/lua52/lua.rs (1)
lua_yieldc(276-278)mlua-sys/src/lua53/lua.rs (1)
lua_yieldc(290-292)
src/serde/mod.rs (2)
src/thread.rs (1)
state(114-116)src/state/util.rs (1)
get_next_spot(403-432)
mlua-sys/src/lua52/lua.rs (2)
mlua-sys/src/lua53/lua.rs (1)
lua_yieldc(290-292)mlua-sys/src/lua54/lua.rs (1)
lua_yieldc(303-305)
src/state/raw.rs (6)
src/thread.rs (3)
state(114-116)new(636-643)status(247-254)src/state/util.rs (6)
callback_error_ext(92-158)callback_error_ext_yieldable(164-330)get_next_spot(403-432)new(19-22)cont_callback(223-244)cont_callback(248-271)src/util/userdata.rs (2)
ffi(32-32)push_internal_userdata(10-38)src/state/extra.rs (2)
new(44-70)lua(269-271)src/util/mod.rs (3)
new(53-58)rawset_field(143-150)check_stack(35-41)src/types/value_ref.rs (1)
clone(51-53)
🔇 Additional comments (43)
src/error.rs (1)
321-325: Fix for missing comma compiles cleanly 👍The trailing comma that was added after the
Error::WithContextarm eliminates the previous compile-error path and keeps the arm formatting consistent with the surrounding code. No further issues spotted.src/lib.rs (1)
113-114: Public re-export looks goodRe-exporting
ContinuationStatusalongsideThread-related types completes the public surface for continuations.mlua-sys/src/lua53/lua.rs (1)
289-292: LGTM – continuation-aware yield helper matches upstream signatureThe new
lua_yieldcwrapper callslua_yieldkwithSome(k), mirroring the
Lua 5.3 C API and the implementations just added for 5.2 and 5.4. No issues spotted.mlua-sys/src/lua52/lua.rs (1)
275-278: LGTM – 5.2 variant correctly useslua_CFunctionImplementation is consistent with Lua 5.2’s continuation type and with the 5.3/5.4
variants. 👍mlua-sys/src/lua54/lua.rs (1)
302-305: LGTM – 5.4 continuation yield helper addedWrapper looks correct (
lua_KFunctionsignature, zero context). Matches other
versions; no further action required.mlua-sys/src/luau/lua.rs (1)
429-432: LGTM! Correct implementation of continuation closure support for Luau.The new
lua_pushcclosurecfunction properly wrapslua_pushcclosurekwith a continuation callback, following the established pattern of other push functions in this file.src/buffer.rs (1)
65-65: Correct usage of auxiliary thread for buffer operations.The change properly passes the auxiliary thread index from the
ValueRefto ensure buffer operations use the correct Lua state.src/util/types.rs (1)
6-7: Proper TypeKey implementation for continuation support.The
ContinuationUpvalueTypeKey implementation correctly follows the established pattern and is appropriately feature-gated for Lua versions that support continuations.Also applies to: 40-47
src/scope.rs (2)
177-178: Consistent use of explicit Lua state for metatable operations.Both changes correctly pass the current Lua state to
push_userdata_metatable_at, aligning with the API changes for explicit state management.Also applies to: 226-227
272-272: Proper auxiliary thread handling in destructor callbacks.All three changes correctly use the auxiliary thread from the
ValueRefto ensure operations are performed on the correct Lua state. This is essential for proper cleanup of scoped values.Also applies to: 292-292, 298-298
tests/tests.rs (2)
1025-1026: Fixed incorrect panic handling in test.The error handling logic was inverted - the test now correctly succeeds when no panic occurs and properly propagates the panic payload when one does occur.
1019-1020:Details
❓ Verification inconclusive
Test performance improvement with debug visibility.
The reduction from 10M to 200K iterations should significantly improve test performance. The debug print helps track progress.
Please verify that 200,000 iterations is still sufficient to trigger ref stack exhaustion on all supported platforms. Run this script to check the ref stack limits:
🏁 Script executed:
#!/bin/bash # Check if there are any constants or configurations related to ref stack size rg -A 5 "ref.*stack|REF.*STACK|reference.*limit|LIMIT.*REF" --type rustLength of output: 6267
🏁 Script executed:
#!/bin/bash # Find the definition of ERROR_TRACEBACK_IDX in the codebase rg -n "ERROR_TRACEBACK_IDX" --type rustLength of output: 334
🏁 Script executed:
#!/bin/bash # Show the test_ref_stack_exhaustion test implementation for context rg -n "fn test_ref_stack_exhaustion" -A 50 --type rustLength of output: 2194
Verify ref-stack exhaustion with reduced loop count
The test intests/tests.rswas shortened from 10 000 000 to 200 000 iterations (now printing progress) to speed up CI. I didn’t find any hard limits in the code (onlyREF_STACK_RESERVEinextra.rsandERROR_TRACEBACK_IDX), so it’s unclear if 200 000 pushes still exhaust the ref stack on every target.
• tests/tests.rs (L1019–1020):for _ in 0..200000 { println!("Creating table {}", vals.len()); vals.push(lua.create_table()?); }Please run this test on all supported platforms to confirm it still triggers the intended ref-stack exhaustion—or bump the iteration count if necessary.
src/value.rs (2)
131-136: LGTM! Correct auxiliary thread handling for string pointers.The change properly uses the auxiliary thread index from the ValueRef to ensure the string is accessed from the correct Lua thread context.
155-164: LGTM! Explicit state parameter for stack operations.The change correctly passes the explicit Lua state pointer to
push_ref_at, ensuring the value is pushed to the intended stack. This aligns with the broader refactoring for explicit thread management.src/serde/mod.rs (1)
184-196: LGTM! Well-structured auxiliary thread management for array metatable.The implementation correctly:
- Acquires a free slot using
get_next_spot- Pushes the array metatable to the main state
- Transfers it to the auxiliary thread using
lua_xmove- Handles slot replacement when necessary
This aligns perfectly with the new multi-threaded reference management architecture.
src/userdata.rs (1)
637-1072: LGTM! Consistent auxiliary thread handling throughout AnyUserData.All changes follow the correct pattern:
lua.ref_thread()→lua.ref_thread(self.0.aux_thread)- Stack operations now use explicit state variants (
push_ref_at,push_at,pop_value_at)This ensures all userdata operations correctly target the auxiliary thread where the userdata reference resides.
src/types.rs (1)
42-57: LGTM! Well-defined continuation types with appropriate feature gates.The new type aliases correctly:
- Feature-gate continuations to Lua versions that support them (5.2+)
- Provide both Send and non-Send variants based on the feature flag
- Match the expected continuation callback signature
The
ContinuationUpvaluetype elegantly combines regular callbacks with continuations.tests/thread.rs (1)
256-268: LGTM! Clean test for multi-value yielding.The test properly validates yielding and resuming with multiple values of different types.
src/types/value_ref.rs (1)
73-83: Good implementation of cross-thread reference comparison.The updated
PartialEqimplementation correctly handles comparisons between references that may exist on different auxiliary threads by using thecompare_refsutility function.src/table.rs (2)
79-81: Consistent update of stack operations to use explicit state pointers.All stack manipulation calls have been systematically updated to use the explicit state pointer variants (
push_ref_at,push_into_specified_stack, etc.). This is necessary and correct for the new multi-auxiliary-thread architecture.Also applies to: 126-128, 156-157, 180-181
409-409: Correct usage of auxiliary thread index for ref_thread access.All
lua.ref_thread()calls now properly include theself.0.aux_threadparameter to ensure operations target the correct auxiliary thread.Also applies to: 464-464, 472-472, 536-536, 544-544, 559-559, 576-576, 758-758
src/state.rs (2)
1299-1335: Well-implemented continuation support for Lua 5.2+.The
create_function_with_continuationmethod properly handles both the main function and continuation callback, with appropriate type conversions and feature gating. The documentation clearly explains the behavior regarding yielding and return values.
1356-1365: Improved C function creation using auxiliary thread slot management.The updated implementation properly uses
get_next_spotto manage auxiliary thread stack slots, ensuring efficient reuse of stack positions and proper thread selection.src/state/extra.rs (1)
35-71: Well-designed RefThread struct for auxiliary thread management.The new
RefThreadstruct properly encapsulates all auxiliary thread-related state and initialization logic. Moving the thread creation and error traceback setup into the constructor improves code organization and reusability.src/thread.rs (2)
29-46: LGTM! Well-structured continuation status enum.The
ContinuationStatusenum and itsfrom_statusmethod provide a clean abstraction for handling continuation thread states.
178-183: Good refactoring to use explicit stack operations.The change to push arguments directly to the thread stack and read results from it eliminates unnecessary stack moves, improving performance.
src/function.rs (2)
499-515: Well-implemented auxiliary thread management in deep_clone.The refactored
deep_clonemethod properly manages auxiliary thread stack slots usingget_next_spotand conditionally replaces the stack slot. This ensures efficient memory usage across multiple auxiliary threads.
117-129: Consistent use of explicit state parameter.All stack operations have been properly updated to use the explicit state parameter, maintaining consistency with the refactoring goals.
src/conversion.rs (2)
83-95: Efficient auxiliary thread management for String conversions.The implementation correctly uses
get_next_spotto manage auxiliary thread stack indices, ensuring efficient memory usage when converting strings from the stack.
794-806: Updated helper function maintains fast path optimization.The
push_bytes_into_stackhelper correctly maintains the fast path for small strings while using the explicit state parameter.src/state/util.rs (3)
31-88: Excellent optimization for error handling.The
PreallocatedFailureenum provides an efficient way to manage error userdata by reusing previously allocated values, reducing memory allocation overhead in error paths.
362-401: Well-designed cross-thread reference comparison.The
compare_refsfunction safely handles references from different auxiliary threads by temporarily pushing them to the internal thread for comparison. The stack management and cleanup are correct.
199-289:Details
✅ Verification successful
Complex but correct yielding implementation.
The yielding logic properly handles the stack state and supports continuations for different Lua versions. The comment about Luau's behavior is particularly helpful for understanding the stack pop requirement.
Verify that the continuation callbacks are properly registered and called:
🏁 Script executed:
#!/bin/bash # Check for tests covering continuation callbacks rg -A 10 "continuation.*test|test.*continuation" tests/Length of output: 1860
Continuation callbacks are fully tested and verified
Tests intests/thread.rs(notablytest_continuation) exercise both non-yieldable and yieldable continuation scenarios, confirming that the callbacks are correctly registered and invoked.
No further changes required.src/traits.rs (2)
21-29: Clean trait extension for explicit state handling.The addition of
push_into_specified_stackmaintains backward compatibility while enabling explicit state control. The delegation pattern in the default implementation is appropriate.
117-136: Consistent multi-value trait extensions.The multi-value trait methods follow the same pattern as single-value methods, maintaining API consistency throughout the codebase.
src/multi.rs (4)
26-36: LGTM!The method signature update correctly adds the explicit Lua state parameter and propagates it to nested calls.
48-58: LGTM!The implementation correctly handles the unit type case while maintaining consistency with the new method signatures.
69-77: LGTM!The generic implementation correctly uses the new
push_into_specified_stackmethod with the explicit state parameter.
344-451: LGTM!The macro-generated tuple implementations correctly follow the new pattern with explicit state parameters. All method calls have been properly updated to use the new signatures.
src/state/raw.rs (4)
625-651: LGTM!The thread recycling implementation correctly handles the new tuple-based storage for auxiliary thread indices. The changes maintain backward compatibility while supporting the multi-thread architecture.
750-829: LGTM!The
stack_value_atimplementation correctly handles all Lua value types with the new auxiliary thread management. The consistent use ofget_next_spotensures proper slot allocation across multiple reference threads.
1472-1476: LGTM!The async callback correctly uses the new explicit state methods for handling multiple return values.
1564-1569: LGTM!The
is_yieldablemethod provides a clean wrapper around Lua's yieldability check for supported versions.
| let lua = self.0.lua.upgrade(); | ||
| let slice = { | ||
| let rawlua = lua.lock(); | ||
| let ref_thread = rawlua.ref_thread(); | ||
| let ref_thread = rawlua.ref_thread(self.0.aux_thread); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify aux_thread bounds before calling ref_thread
rawlua.ref_thread(self.0.aux_thread) relies on the index being in-range; inside ref_thread an unwrap_unchecked is used, so an out-of-bounds value is instant UB.
Even though higher-level code is expected to uphold this invariant, adding a debug assertion here would catch logic errors early without runtime cost in release builds.
+ debug_assert!(
+ self.0.aux_thread < rawlua.ref_thread_count(),
+ "aux_thread index {} out of bounds",
+ self.0.aux_thread
+ );
let ref_thread = rawlua.ref_thread(self.0.aux_thread);(Replace ref_thread_count() with the actual accessor if it exists.)
🏁 Script executed:
#!/bin/bash
# Search for ref_thread definition and usage
rg -n "fn ref_thread" .
rg -n "ref_thread(" .
# Locate RawLua struct to inspect its fields and any count methods
rg -n "pub struct RawLua" .
# Search for aux_thread usage to identify its type and origin
rg -n "aux_thread" .Length of output: 11540
🏁 Script executed:
#!/bin/bash
# Print RawLua struct definition and any related fields to inspect thread storage
sed -n '1,200p' src/state/raw.rs | sed -n '1,200p'Length of output: 6410
🏁 Script executed:
#!/bin/bash
# Locate and print the ExtraData struct to inspect the ref_thread field and its type
sed -n '1,200p' src/state/extra.rsLength of output: 6985
Add debug‐assert in RawLua::ref_thread to catch invalid aux_thread indices
Centralize the bounds check in src/state/raw.rs so every call to ref_thread is guarded in debug builds without touching each call site:
• File: src/state/raw.rs
• Location: inside impl RawLua, method ref_thread
impl RawLua {
#[inline(always)]
pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State {
+ debug_assert!(
+ aux_thread < unsafe { (*self.extra.get()).ref_thread.len() },
+ "aux_thread index {} out of bounds (len={})",
+ aux_thread,
+ unsafe { (*self.extra.get()).ref_thread.len() }
+ );
unsafe {
(*self.extra.get())
.ref_thread
.get(aux_thread)
.unwrap_unchecked()
.ref_thread
}
}This ensures any out-of-range aux_thread is caught during development (zero cost in release) and prevents the UB from unwrap_unchecked.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/state/raw.rs inside the impl block for RawLua, add a debug_assert in the
ref_thread method to verify that the aux_thread index is within valid bounds
before calling unwrap_unchecked. This will catch out-of-range indices during
development without affecting release builds, preventing undefined behavior from
invalid indexing. Centralizing this check here avoids modifying every call site
like in src/string.rs lines 119-123.
tests/byte_string.rs
Outdated
| //#[test] | ||
| fn test_byte_string_round_trip() -> Result<()> { |
There was a problem hiding this comment.
Accidentally disabled a valuable round-trip test
The former test_byte_string_round_trip was commented out (//#[test]). That test validated several tricky UTF-8 edge-cases and its loss is a regression in coverage. If it has started failing, mark it #[ignore] with a TODO instead of disabling it entirely, so the failure is still visible.
-//#[test]
+#[test]
+#[ignore = "fails after Threadopt refactor – investigate and re-enable"]🤖 Prompt for AI Agents
In tests/byte_string.rs around lines 11 to 12, the test function
test_byte_string_round_trip is commented out, disabling an important test that
covers tricky UTF-8 edge cases. Instead of commenting it out, restore the
#[test] attribute and add #[ignore] above it with a TODO comment explaining why
it is ignored, so the test remains in the suite but is skipped, keeping the
failure visible for future fixing.
| pub fn yield_with(&self, args: impl IntoLuaMulti) -> Result<()> { | ||
| let raw = self.lock(); | ||
| unsafe { | ||
| raw.extra.get().as_mut().unwrap_unchecked().yielded_values = Some(args.into_lua_multi(self)?); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Safety concern with unchecked pointer access.
The use of unwrap_unchecked() on line 2182 could cause undefined behavior if extra.get() returns null. Consider using a checked operation or documenting why this is guaranteed to be safe.
pub fn yield_with(&self, args: impl IntoLuaMulti) -> Result<()> {
let raw = self.lock();
unsafe {
- raw.extra.get().as_mut().unwrap_unchecked().yielded_values = Some(args.into_lua_multi(self)?);
+ match raw.extra.get().as_mut() {
+ Some(extra) => extra.yielded_values = Some(args.into_lua_multi(self)?),
+ None => return Err(Error::MismatchedRegistryKey), // or appropriate error
+ }
}
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn yield_with(&self, args: impl IntoLuaMulti) -> Result<()> { | |
| let raw = self.lock(); | |
| unsafe { | |
| raw.extra.get().as_mut().unwrap_unchecked().yielded_values = Some(args.into_lua_multi(self)?); | |
| } | |
| Ok(()) | |
| } | |
| pub fn yield_with(&self, args: impl IntoLuaMulti) -> Result<()> { | |
| let raw = self.lock(); | |
| unsafe { | |
| match raw.extra.get().as_mut() { | |
| Some(extra) => extra.yielded_values = Some(args.into_lua_multi(self)?), | |
| None => return Err(Error::MismatchedRegistryKey), // or use an appropriate error variant | |
| } | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In src/state.rs around lines 2179 to 2185, the use of unwrap_unchecked() on the
pointer returned by extra.get() is unsafe and could cause undefined behavior if
the pointer is null. Replace unwrap_unchecked() with a safe checked operation
like unwrap() or expect() to handle the null case explicitly, or add a comment
explaining why the pointer is guaranteed to be non-null at this point to justify
the unsafe usage.
| pub(crate) unsafe fn get_next_spot(extra: *mut ExtraData) -> (usize, c_int, bool) { | ||
| let extra = &mut *extra; | ||
|
|
||
| // Find the first thread with a free slot | ||
| for (i, ref_th) in extra.ref_thread.iter_mut().enumerate() { | ||
| if let Some(free) = ref_th.free.pop() { | ||
| return (i, free, true); | ||
| } | ||
| extra.ref_stack_size += inc; | ||
|
|
||
| // Try to grow max stack size | ||
| if ref_th.stack_top >= ref_th.stack_size { | ||
| let mut inc = ref_th.stack_size; // Try to double stack size | ||
| while inc > 0 && ffi::lua_checkstack(ref_th.ref_thread, inc + 1) == 0 { | ||
| inc /= 2; | ||
| } | ||
| if inc == 0 { | ||
| continue; // No stack space available, try next thread | ||
| } | ||
| ref_th.stack_size += inc; | ||
| } | ||
|
|
||
| ref_th.stack_top += 1; | ||
| return (i, ref_th.stack_top, false); | ||
| } | ||
| extra.ref_stack_top += 1; | ||
| extra.ref_stack_top | ||
|
|
||
| // No free slots found, create a new one | ||
| let new_ref_thread = RefThread::new(extra.raw_lua().state()); | ||
| extra.ref_thread.push(new_ref_thread); | ||
| return get_next_spot(extra); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify thread creation doesn't cause memory issues.
The get_next_spot function creates new auxiliary threads when all existing ones are full. Ensure there are reasonable limits to prevent unbounded thread creation.
🏁 Script executed:
#!/bin/bash
# Check if there are any limits on auxiliary thread creation
rg -A 5 "ref_thread.*push|RefThread::new" src/Length of output: 1024
Enforce a maximum auxiliary thread count
Currently, get_next_spot will keep allocating new RefThread instances without any bounds, which could lead to unbounded memory growth under sustained load. You should introduce and check against a configurable cap before calling RefThread::new().
• In src/state/util.rs inside get_next_spot:
- Before
let new_ref_thread = RefThread::new(...), verify thatextra.ref_thread.len() < MAX_AUX_THREADS. - If the limit is reached, return an error (or propagate a failure) instead of recursing.
• Define MAX_AUX_THREADS (e.g. in src/state/extra.rs or a new config module) and document its rationale.
This will prevent runaway thread creation and potential out-of-memory crashes.
🤖 Prompt for AI Agents
In src/state/util.rs between lines 403 and 432, the get_next_spot function
currently creates new RefThread instances without any limit, risking unbounded
memory growth. Introduce a constant MAX_AUX_THREADS (defined in
src/state/extra.rs or a new config module with documentation) and before
creating a new RefThread, check if extra.ref_thread.len() is less than
MAX_AUX_THREADS. If the limit is reached, return an error or propagate failure
instead of recursing to prevent runaway thread creation.
| pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State { | ||
| unsafe { | ||
| (*self.extra.get()) | ||
| .ref_thread | ||
| .get(aux_thread) | ||
| .unwrap_unchecked() | ||
| .ref_thread | ||
| } | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| #[cfg(any(feature = "lua51", feature = "luajit", feature = "luau"))] | ||
| pub(crate) fn ref_thread_internal(&self) -> *mut ffi::lua_State { | ||
| unsafe { (*self.extra.get()).ref_thread_internal.ref_thread } | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate aux_thread index before unsafe access.
The ref_thread method uses unwrap_unchecked on a get operation without bounds checking. This could lead to undefined behavior if aux_thread is out of bounds.
Consider adding bounds checking or documenting the safety invariant:
pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State {
unsafe {
- (*self.extra.get())
- .ref_thread
- .get(aux_thread)
- .unwrap_unchecked()
- .ref_thread
+ let ref_threads = &(*self.extra.get()).ref_thread;
+ debug_assert!(aux_thread < ref_threads.len(), "aux_thread index out of bounds");
+ ref_threads
+ .get(aux_thread)
+ .unwrap_unchecked()
+ .ref_thread
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State { | |
| unsafe { | |
| (*self.extra.get()) | |
| .ref_thread | |
| .get(aux_thread) | |
| .unwrap_unchecked() | |
| .ref_thread | |
| } | |
| } | |
| #[inline(always)] | |
| #[cfg(any(feature = "lua51", feature = "luajit", feature = "luau"))] | |
| pub(crate) fn ref_thread_internal(&self) -> *mut ffi::lua_State { | |
| unsafe { (*self.extra.get()).ref_thread_internal.ref_thread } | |
| } | |
| pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State { | |
| unsafe { | |
| let ref_threads = &(*self.extra.get()).ref_thread; | |
| debug_assert!(aux_thread < ref_threads.len(), "aux_thread index out of bounds"); | |
| ref_threads | |
| .get(aux_thread) | |
| .unwrap_unchecked() | |
| .ref_thread | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/state/raw.rs around lines 128 to 143, the ref_thread method uses
unwrap_unchecked on a get call without verifying that aux_thread is within
bounds, risking undefined behavior. To fix this, add explicit bounds checking
for aux_thread before accessing the vector, returning an Option or Result to
handle out-of-bounds safely, or clearly document the safety invariant ensuring
aux_thread is always valid when this method is called.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/state.rs (1)
2186-2189:⚠️ Potential issue
unwrap_unchecked()safety issue remains unresolvedThe previous review already highlighted that
extra.get()can legally be
null for externally-owned states. Dereferencing it with
unwrap_unchecked()causes UB if that happens. The same code is still
present inyield_with.-raw.extra.get().as_mut().unwrap_unchecked().yielded_values = Some(...); +match raw.extra.get().as_mut() { + Some(extra) => extra.yielded_values = Some(...), + None => return Err(Error::MismatchedRegistryKey), // or a better error +}Please replace the unchecked unwrap or document the invariants that make
this safe.
🧹 Nitpick comments (5)
src/userdata/ref.rs (2)
83-85: Document the new safety guarantees offrom_specified_stackThe function now accepts a raw
statepointer. Call-sites must guarantee that
statebelongs to the same Lua instance represented bylua; otherwise UB ensues.
Please add a short# Safetysection to the doc-comment (or a regular comment if you
prefer) spelling this out.+ /// # Safety + /// `state` must be the same Lua state associated with `lua`. + /// Passing a pointer to an unrelated or already-closed state is undefined behaviour. unsafe fn from_specified_stack(idx: c_int, lua: &RawLua, state: *mut ffi::lua_State) -> Result<Self> {
298-300: Same safety note applies to the mutable variantReplicate the safety documentation here to keep the two implementations consistent.
src/userdata/registry.rs (3)
247-249: Minor perf/readability nit – avoid callingraw_lua()twiceInside the async block the same raw Lua handle is computed twice:
lua.raw_lua(), lua.raw_lua().state()Cache it once:
- fut.await? - .push_into_specified_stack_multi(lua.raw_lua(), lua.raw_lua().state()) + let raw = lua.raw_lua(); + fut.await?.push_into_specified_stack_multi(raw, raw.state())
290-293: Repeat of the raw-handle caching nitConsider applying the same single-call optimisation as suggested above.
339-348: Async function wrapper – same raw handle caching nitThe duplicate
lua.raw_lua()call pattern appears here as well; consider
deduplicating for clarity and micro-efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/function.rs(16 hunks)src/luau/mod.rs(1 hunks)src/scope.rs(5 hunks)src/state.rs(20 hunks)src/table.rs(25 hunks)src/thread.rs(8 hunks)src/traits.rs(4 hunks)src/userdata.rs(15 hunks)src/userdata/ref.rs(2 hunks)src/userdata/registry.rs(9 hunks)tests/byte_string.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/byte_string.rs
- src/luau/mod.rs
- src/userdata.rs
- src/scope.rs
- src/table.rs
- src/thread.rs
- src/function.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/userdata/ref.rs (3)
src/traits.rs (1)
from_specified_stack(55-57)src/state/raw.rs (3)
lua(103-105)lua(1515-1515)state(116-118)src/thread.rs (1)
state(114-116)
src/state.rs (7)
src/thread.rs (4)
state(114-116)new(634-641)status(245-252)from_status(39-45)src/state/raw.rs (8)
state(116-118)lua(103-105)lua(1515-1515)ref_thread_internal(140-142)extra(145-147)new(149-205)new(1589-1593)is_yieldable(1566-1568)src/state/util.rs (2)
get_next_spot(403-432)new(19-22)src/conversion.rs (8)
from_specified_stack(83-95)from_specified_stack(133-139)from_specified_stack(177-183)from_specified_stack(418-420)from_specified_stack(532-548)from_specified_stack(664-683)from_specified_stack(1154-1159)from_specified_stack(1200-1215)src/util/mod.rs (2)
check_stack(35-41)new(53-58)src/traits.rs (3)
from_specified_stack_multi(136-146)from_specified_stack_args(151-165)from_specified_stack(55-57)src/state/extra.rs (1)
new(44-70)
🔇 Additional comments (8)
src/userdata/registry.rs (8)
136-136: Updated call matches the new API – looks good
Nothing to flag; the explicit state propagation is correct.
143-150: Propagation topush_into_specified_stack_multiverifiedBoth shared and unique-pointer branches now forward the explicit
state
pointer; this keeps stack ownership clear.
185-185: Correct argument extraction for mutable method wrapper
from_specified_stack_argsis invoked with the right offsets after moving the
“self” parameter—nice.
192-199: Mutable borrow path updated consistentlyThe mutable branch mirrors the read-only one; no further comments.
235-237: Async wrapper correctly keepsselfalive on the ref threadSwitching to
AnyUserData::from_specified_stackwith-nargsis the right
choice.
279-281: Async-mut wrapper: argument extraction looks correctSame pattern as the immutable version; no concerns.
305-308: Function wrapper migration is correct
from_specified_stack_args+push_into_specified_stack_multipair up as
expected.
323-326: Mutable function wrapper migration is correctNo issues observed.
| let (aux_thread, idx, replace) = get_next_spot(lua.extra()); | ||
| ffi::lua_pushcfunction(lua.ref_thread(aux_thread), func); | ||
| if replace { | ||
| ffi::lua_replace(lua.ref_thread(aux_thread), idx); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing stack-space / OOM handling around lua_pushcfunction
lua_pushcfunction allocates on the target stack.
Unlike most other helpers in this module, this path does not:
check_stack(...)before the push, nor- fall back to the
unlikely_memory_errorbranch.
An allocation failure here will raise a Lua error that bypasses the Rust
error path and can leave the ref-thread in an inconsistent state.
+// ensure at least one free slot on the ref-thread stack
+check_stack(lua.ref_thread(aux_thread), 1)?;
ffi::lua_pushcfunction(lua.ref_thread(aux_thread), func);Consider adding the usual protect_lua! wrapper as well.
🏁 Script executed:
#!/bin/bash
# Locate all uses of lua_pushcfunction in the codebase
rg -n "lua_pushcfunction" -n src
# Locate all check_stack calls to see how they are used elsewhere
rg -n "check_stack" -n src
# Locate any protect_lua! macro invocations
rg -n "protect_lua!" -n srcLength of output: 11752
Add stack-space check and error protection around lua_pushcfunction
The push into the ref-thread stack at src/state.rs:1361–1365 isn’t guarded by check_stack or wrapped in protect_lua!, so an OOM long-jmp could bypass Rust error handling and corrupt the ref-thread’s state.
• File: src/state.rs
Location: around line 1362
Suggested diff:
let (aux_thread, idx, replace) = get_next_spot(lua.extra());
+ // ensure at least one free slot on the ref-thread stack to avoid OOM long-jmp
+ check_stack(lua.ref_thread(aux_thread), 1)?;
ffi::lua_pushcfunction(lua.ref_thread(aux_thread), func);
if replace {
ffi::lua_replace(lua.ref_thread(aux_thread), idx);
}You should also consider wrapping this block in the standard protect_lua!(…) macro to catch allocation failures and translate them into Rust Result errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (aux_thread, idx, replace) = get_next_spot(lua.extra()); | |
| ffi::lua_pushcfunction(lua.ref_thread(aux_thread), func); | |
| if replace { | |
| ffi::lua_replace(lua.ref_thread(aux_thread), idx); | |
| } | |
| let (aux_thread, idx, replace) = get_next_spot(lua.extra()); | |
| // ensure at least one free slot on the ref-thread stack to avoid OOM long-jmp | |
| check_stack(lua.ref_thread(aux_thread), 1)?; | |
| ffi::lua_pushcfunction(lua.ref_thread(aux_thread), func); | |
| if replace { | |
| ffi::lua_replace(lua.ref_thread(aux_thread), idx); | |
| } |
🤖 Prompt for AI Agents
In src/state.rs around lines 1361 to 1365, the call to lua_pushcfunction is
missing a preceding check_stack call and is not wrapped in the protect_lua!
macro. To fix this, add a check_stack call before lua_pushcfunction to ensure
sufficient stack space, and wrap the lua_pushcfunction call and the subsequent
lua_replace call inside the protect_lua! macro to properly handle allocation
failures and prevent Lua errors from bypassing Rust error handling.
| let values = self.into_lua_multi(lua.lua())?; | ||
| let len: c_int = values.len().try_into().unwrap(); | ||
| unsafe { | ||
| check_stack(lua.state(), len + 1)?; | ||
| check_stack(state, len + 1)?; | ||
| for val in &values { | ||
| lua.push_value(val)?; | ||
| lua.push_value_at(val, state)?; | ||
| } | ||
| } | ||
| Ok(len) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential panic & off-by-one in push_into_specified_stack_multi
-
values.len().try_into().unwrap()will panic when more than
i32::MAXelements are pushed. This is rare but avoidable. -
check_stack(state, len + 1)?reserves one extra slot although only
lenpushes follow. Unless something else is pushed later, use
lento keep the check tight.
-let len: c_int = values.len().try_into().unwrap();
+let len: c_int = match c_int::try_from(values.len()) {
+ Ok(n) => n,
+ Err(_) => return Err(Error::RuntimeError("too many values to push".into())),
+};
-check_stack(state, len + 1)?;
+check_stack(state, len)?;This removes a possible panic path and keeps stack checks accurate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let values = self.into_lua_multi(lua.lua())?; | |
| let len: c_int = values.len().try_into().unwrap(); | |
| unsafe { | |
| check_stack(lua.state(), len + 1)?; | |
| check_stack(state, len + 1)?; | |
| for val in &values { | |
| lua.push_value(val)?; | |
| lua.push_value_at(val, state)?; | |
| } | |
| } | |
| Ok(len) | |
| let values = self.into_lua_multi(lua.lua())?; | |
| let len: c_int = match c_int::try_from(values.len()) { | |
| Ok(n) => n, | |
| Err(_) => return Err(Error::RuntimeError("too many values to push".into())), | |
| }; | |
| unsafe { | |
| check_stack(state, len)?; | |
| for val in &values { | |
| lua.push_value_at(val, state)?; | |
| } | |
| } | |
| Ok(len) |
🤖 Prompt for AI Agents
In src/traits.rs around lines 96 to 104, avoid using unwrap on try_into for
values.len() to prevent panic when the length exceeds i32::MAX by handling the
conversion safely. Also, change check_stack(state, len + 1)? to
check_stack(state, len)? to accurately reserve stack space matching the number
of pushed values, removing the unnecessary extra slot.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/conversion.rs (1)
133-139: 🛠️ Refactor suggestion
mem::transmuteto'staticrisks future unsoundness
let buf = unsafe { mem::transmute::<&str, &'static str>(buf) };(and the analogous version for[u8]) relies onCow::Owned(s)to keep the underlyingStringalive.
Although this is currently correct, any future refactor that replacesCow::Ownedwith an ownedStringstored elsewhere (or dropsborrowbeforebuf) will silently introduce UB.A safer pattern:
- let buf = unsafe { mem::transmute::<&str, &'static str>(buf) }; + // Extend the lifetime without lying to the compiler + let buf: &'static str = Box::leak(buf.to_owned().into_boxed_str());The extra allocation is rarely performance-critical and removes the foot-gun.
🧹 Nitpick comments (1)
src/conversion.rs (1)
1206-1211: Loss-less error handling preferred over unconditionalunwrap()
CStr::from_ptr(ffi::luaL_typename(state, idx)).to_str().unwrap()will panic on invalid UTF-8, turning a recoverable conversion error into an abort. Replace with fallible handling:- let value_type_name = CStr::from_ptr(ffi::luaL_typename(state, idx)); - Err(Error::FromLuaConversionError { - from: value_type_name.to_str().unwrap(), + let raw = CStr::from_ptr(ffi::luaL_typename(state, idx)); + let from = raw.to_str().unwrap_or("<non-utf8>"); + Err(Error::FromLuaConversionError { + from,This keeps the API purely error-driven and avoids unexpected panics in release builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/conversion.rs(34 hunks)src/thread.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/thread.rs
🔇 Additional comments (1)
src/conversion.rs (1)
83-95:Details
❌ Incorrect review comment
Possible stack-imbalance when cloning a string into an auxiliary thread
ffi::lua_xpush(state, lua.ref_thread(aux_thread), idx)duplicates the value from the origin stack (state) onto an auxiliary thread’s stack, but the source slot onstateis left untouched.
If the caller expects the value atidxto be consumed (similar tolua_remove/lua_replacesemantics used a few lines later), this leaves an extra copy on the caller’s stack and silently shifts subsequent absolute indices.Please double-check:
- Whether the value at
idxmust be removed (e.g.ffi::lua_remove(state, idx)after the x-push), or- Whether the documentation of
from_specified_stackexplicitly guarantees “no pop”.A silent stack offset is extremely hard to debug.
🏁 Script executed:
#!/bin/bash # Search for definition and usage of lua_xpush to confirm its stack behavior rg -nC3 "lua_xpush" .Length of output: 11971
🏁 Script executed:
#!/bin/bash # Show the definition and any doc comments of from_specified_stack in src/conversion.rs rg -nC5 "from_specified_stack" -n src/conversion.rsLength of output: 5135
#!/bin/bash # Locate the IntoLua trait definition and its documentation rg -nC5 "trait IntoLua" -n src
No stack imbalance –
lua_xpushpreserves the original stackAfter checking the mlua-sys definitions,
ffi::lua_xpush(from, to, idx)expands to:lua_pushvalue(from, idx); lua_xmove(from, to, 1);The temporary copy pushed onto
fromis immediately moved off withlua_xmove, leaving the original stack exactly as it was. There is no extra value left onstate, so no call tolua_removeorlua_replaceis needed here.Likely an incorrect or invalid review comment.
| #[inline] | ||
| unsafe fn push_bytes_into_stack<T>(this: T, lua: &RawLua) -> Result<()> | ||
| unsafe fn push_bytes_into_stack<T>(this: T, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()> | ||
| where | ||
| T: IntoLua + AsRef<[u8]>, | ||
| { | ||
| let bytes = this.as_ref(); | ||
| if lua.unlikely_memory_error() && bytes.len() < (1 << 30) { | ||
| // Fast path: push directly into the Lua stack. | ||
| ffi::lua_pushlstring(lua.state(), bytes.as_ptr() as *const _, bytes.len()); | ||
| ffi::lua_pushlstring(state, bytes.as_ptr() as *const _, bytes.len()); | ||
| return Ok(()); | ||
| } | ||
| // Fallback to default | ||
| lua.push_value(&T::into_lua(this, lua.lua())?) | ||
| lua.push_value_at(&T::into_lua(this, lua.lua())?, state) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
push_bytes_into_stack is overly restrictive & may fail for &str
T: AsRef<[u8]> excludes common callers such as &str (which only implements AsRef<str>).
As written, unsafe fn push_bytes_into_stack::<&str>(...) will not compile.
A minimal, non-breaking fix:
-trait IntoLua + AsRef<[u8]>,
+trait IntoLua,
+where for<'a> &'a T: AsRef<[u8]>,or simply accept any AsRef<[u8]> via an extra generic:
fn push_bytes_into_stack<B>(bytes: B, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()>
where
B: AsRef<[u8]>,and call it with this.as_bytes() for the String/&str impls.
This avoids surprising compilation failures when the refactor is consumed down-stream.
🏁 Script executed:
#!/usr/bin/env bash
# Locate the IntoLua trait definition
rg "trait IntoLua" -n
# List all impls of IntoLua to see which types are supported
rg "impl\s+IntoLua" -n
# Check specifically for &str and String implementations
rg "impl\s+IntoLua\s+for\s+&str" -n
rg "impl\s+IntoLua\s+for\s+String" -n
# Find all AsRef<[u8]> impls in the codebase
rg "impl\s+AsRef<\s*\[u8\]\s*>" -n
# Verify if &str implements AsRef<[u8]>
rg "impl\s+AsRef<\s*\[u8\]\s*>\s+for\s+&str" -nLength of output: 10344
🏁 Script executed:
#!/usr/bin/env bash
# Display the implementation of IntoLua for &str to see how push_bytes_into_stack is used
sed -n '530,580p' src/conversion.rsLength of output: 1624
Loosen push_bytes_into_stack bound to support &str
The current helper in src/conversion.rs (~lines 793–806):
#[inline]
unsafe fn push_bytes_into_stack<T>(this: T, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()>
where
T: IntoLua + AsRef<[u8]>,
{ … }requires T: AsRef<[u8]>, which excludes &str (it only implements AsRef<str>). Yet the IntoLua impl for &str calls it directly:
unsafe fn push_into_specified_stack(self, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()> {
push_bytes_into_stack(self, lua, state) // ← fails: &str ≠ AsRef<[u8]>
}To resolve, choose one of these non-breaking fixes:
-
Refactor the helper to a standalone
AsRef<[u8]>generic and update call sites:#[inline]
-
unsafe fn push_bytes_into_stack(this: T, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()>
-
where T: IntoLua + AsRef<[u8]>,
- unsafe fn push_bytes_into_stack<B: AsRef<[u8]>>(bytes: B, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()>
{
let bytes = bytes.as_ref();
…
}
And in `impl IntoLua for &str` (and `String`/`Box<str>` etc.):
```diff
unsafe fn push_into_specified_stack(self, lua: &RawLua, state: *mut ffi::lua_State) -> Result<()> {
- push_bytes_into_stack(self, lua, state)
+ push_bytes_into_stack(self.as_bytes(), lua, state)
}
-
Or add an
AsRef<[u8]>impl for string slices so the original bound still works:impl AsRef<[u8]> for str { #[inline] fn as_ref(&self) -> &[u8] { self.as_bytes() } }
Please update src/conversion.rs accordingly.
🤖 Prompt for AI Agents
In src/conversion.rs around lines 793 to 806, the function push_bytes_into_stack
is defined with a generic bound T: IntoLua + AsRef<[u8]>, which excludes &str
because &str implements AsRef<str> but not AsRef<[u8]>. To fix this, refactor
push_bytes_into_stack to accept a generic parameter B: AsRef<[u8]> instead of T,
and update call sites to pass this.as_bytes() when calling it from &str or
String implementations. Alternatively, add an impl AsRef<[u8]> for str that
returns self.as_bytes(), allowing the original bound to work. Apply one of these
fixes to support &str without breaking existing code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chunk.rs (1)
651-665: Unbounded cache may lead to memory bloat
ChunksCachegrows for every distinct source string and is never pruned. A long-running host that dynamically loads user scripts can leak megabytes of bytecode.Consider an eviction strategy, e.g. wrap the
HashMapin anlru::LruCachewith a configurable capacity:-struct ChunksCache(HashMap<Vec<u8>, Vec<u8>>); +use lru::LruCache; + +/// (capacity chosen arbitrarily – make this configurable) +const DEFAULT_CACHE_ENTRIES: usize = 256; + +struct ChunksCache(LruCache<Vec<u8>, Vec<u8>>);and initialise:
-let mut cache = ChunksCache(HashMap::new()); +let mut cache = ChunksCache(LruCache::new(DEFAULT_CACHE_ENTRIES));This keeps memory usage predictable while preserving most of the speed benefit.
| /// | ||
| /// If not found, compiles the source code and stores it on the cache. | ||
| pub(crate) fn try_cache(mut self) -> Self { | ||
| pub fn try_cache(mut self) -> Self { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public exposure of try_cache lacks safety & lifecycle guarantees
Changing the visibility from pub(crate) to pub exposes a method that:
- Relies on the
*_unguardedapp-data APIs, i.e. assumes the caller is not in a re-entrant Lua context. - Mutates an unbounded, process-global
HashMapwithout any eviction strategy. - Consumes and returns
self, which is fine in fluent chains but surprises if a caller tries to re-use the originalChunk.
Before stabilising the API, please document:
• When it is safe to call this from user code (thread-safety, re-entrancy).
• The fact that the cache is per-Lua instance and unbounded.
• That the method is intended for one-shot, builder-style usage.
Alternatively, keep it pub(crate) and expose a higher-level builder (with_cache(true)) that can enforce invariants internally.
This will confirm whether try_cache is truly public, relies on “unguarded” APIs, and mutates a global, unbounded cache.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== 1. Definition and immediate context of try_cache ==="
rg -n "pub fn try_cache" -C5 src/chunk.rs || true
echo
echo "=== 2. Look for unguarded app-data API calls ==="
rg -n "unguarded" -C5 src || true
echo
echo "=== 3. Examine cache storage (HashMap / static) in src/chunk.rs ==="
rg -n "HashMap" -C5 src/chunk.rs || true
rg -n "static" -C5 src/chunk.rs || trueLength of output: 6803
Public API try_cache needs safety docs or scoped visibility
The newly public try_cache method:
- Calls
app_data_ref_unguarded/app_data_mut_unguarded, which assume no re-entrant Lua calls and have no locking or re-entrancy checks. - Stores a
HashMap<Vec<u8>,Vec<u8>>in the Lua state’s app-data; it’s unbounded and per-Lua-instance (not process global) with no eviction strategy. - Takes ownership of
selfand returns it, which works for one-shot builder chains but will confuse callers re-using the originalChunk.
Before exposing this as stable public API, either:
• Add precise documentation on
- When it’s safe to call (
threading/re-entrancy guarantees), - Cache scope (per-
Luainstance, unbounded, no eviction), - Usage pattern (one-shot, builder-style only).
—or—
• Keeptry_cacheaspub(crate)and offer a higher-levelwith_cache(true)builder method that enforces these invariants internally.
🤖 Prompt for AI Agents
In src/chunk.rs at line 632, the method try_cache was changed from pub(crate) to
pub but it uses unsafe unguarded app-data APIs, mutates an unbounded
per-Lua-instance cache, and consumes self which can confuse callers. To fix
this, either revert try_cache to pub(crate) and create a new public builder
method like with_cache(true) that enforces safety and usage invariants
internally, or keep it public but add detailed documentation specifying when it
is safe to call (threading and re-entrancy constraints), the cache scope and
limitations (per-Lua instance, unbounded, no eviction), and that it is intended
for one-shot builder-style usage only.
The old one is still supported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/state/raw.rs (1)
128-136:⚠️ Potential issue
ref_threadstill usesunwrap_uncheckedwithout bounds validationThe earlier review already pointed out that blindly calling
unwrap_uncheckedon theaux_threadindex is UB if the index is ever stale or corrupted.
The implementation is unchanged, so the safety hole remains.pub(crate) fn ref_thread(&self, aux_thread: usize) -> *mut ffi::lua_State { unsafe { - (&(*self.extra()) - .ref_thread) - .get(aux_thread) - .unwrap_unchecked() - .ref_thread + let ref_threads = &(*self.extra()).ref_thread; + debug_assert!( + aux_thread < ref_threads.len(), + "aux_thread index {aux_thread} out of bounds ({})", + ref_threads.len() + ); + // SAFETY: `aux_thread` has been bounds-checked above. + ref_threads.get_unchecked(aux_thread).ref_thread } }Even a
debug_assert!would prevent accidental UB in debug builds; returning anOption/Resultwould be safer still.
🧹 Nitpick comments (1)
src/state/raw.rs (1)
1290-1355: Large duplication between Luau and classic continuation pathsThe Luau and “plain Lua 5.2+” branches of
create_callback_with_continuationare ~95 % identical, differing only in thepushcclosure{c}call and the continuation signature.
This inflates maintenance cost and obscures the true differences.A small macro or internal helper that accepts the two call-sites as parameters would reduce ~60 lines of near-identical unsafe code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/state/raw.rs(28 hunks)
🔇 Additional comments (1)
src/state/raw.rs (1)
626-638: Re-using pooled threads without liveness verificationWhen a thread is popped from
thread_pool, the code assumes the underlying coroutine is still valid:let thread_state = ffi::lua_tothread(self.ref_thread(aux_thread), index);If the coroutine has terminated and been GC-collected,
lua_tothreadmay return null, or the slot might now contain a different value.
Consider:
- Verifying
lua_status(thread_state) == LUA_OK || LUA_YIELDbefore reuse.- Falling back to
create_threadif the check fails.- Purging invalid entries from the pool.
Would you run a quick audit/test to confirm the pool never hands back dead coroutines?
| let ref_thread = self.ref_thread(vref.aux_thread); | ||
| mlua_debug_assert!( | ||
| ffi::lua_gettop(ref_thread) >= vref.index, | ||
| "GC finalizer is not allowed in ref_thread" | ||
| ); | ||
| ffi::lua_pushnil(ref_thread); | ||
| ffi::lua_replace(ref_thread, vref.index); | ||
| (*self.extra.get()).ref_free.push(vref.index); | ||
| (&mut (*self.extra.get()).ref_thread)[vref.aux_thread] | ||
| .free | ||
| .push(vref.index); | ||
| } |
There was a problem hiding this comment.
Unchecked vector access in drop_ref mirrors the same UB risk
drop_ref repeats the pattern of unchecked indexing into ref_thread, this time mutably:
(&mut (*self.extra.get()).ref_thread)[vref.aux_thread]If vref.aux_thread is out of range the program instantly hits undefined behaviour.
Please guard the access or (better) wrap it in a helper that enforces the invariant once and for all.
🤖 Prompt for AI Agents
In src/state/raw.rs around lines 887 to 897, the mutable indexing into
ref_thread using vref.aux_thread is unchecked and can cause undefined behavior
if out of range. To fix this, add a bounds check before accessing ref_thread or
create a helper method that safely accesses ref_thread with proper invariant
enforcement. Replace the direct indexing with this safe helper to prevent
potential UB.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/state/raw.rs (2)
128-135: Uncheckedunwrap_uncheckedis still a latent UB source
The earlier review already pointed this out; the risk is unchanged.
Please add a bound-check (or a debug‐assert with a documented safety invariant) before callingunwrap_unchecked, or return anOption/Result.
886-895:drop_refmirrors the same unchecked indexing issueThe direct
ref_thread[vref.aux_thread]access is still unchecked and can trigger undefined behaviour if the index is stale or corrupted. The helper suggested in the earlier review would fix both call-sites in one place.
🧹 Nitpick comments (1)
src/state/raw.rs (1)
15-17: Gate the import instead of silencing the warningUsing
#[allow(unused_imports)]hides potentially useful compiler feedback.
Prefer to wrap thecallback_error_extimport in the samecfgthat guards its actual use instead of globally suppressing the warning.-#[allow(unused_imports)] -use crate::state::util::callback_error_ext; +#[cfg(any(not(feature = "lua51"), not(feature = "luajit"), feature = "luau"))] // adjust as appropriate +use crate::state::util::callback_error_ext;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/state/raw.rs(28 hunks)
🔇 Additional comments (1)
src/state/raw.rs (1)
625-637: Validate recycled thread slots before reuse
thread_pool.pop()may return a(aux_thread, index)pair that has since been overwritten or collected.
Consider:
- Verifying that
ffi::lua_type(self.ref_thread(aux_thread), index) == ffi::LUA_TTHREADbefore using it.- Dropping the pair if validation fails and falling back to
create_thread.This avoids propagating dangling
lua_State*pointers.
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests