Skip to content
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

Thread into_async should be generic on args #508

Open
cheesycod opened this issue Jan 6, 2025 · 9 comments
Open

Thread into_async should be generic on args #508

cheesycod opened this issue Jan 6, 2025 · 9 comments

Comments

@cheesycod
Copy link

cheesycod commented Jan 6, 2025

Right now, Thread::into_async is not really usable because it returns a AsyncThread<impl IntoLuaMulti, R> where the impl IntoLuaMulti is the same as A:

    pub fn into_async<R>(self, args: impl IntoLuaMulti) -> AsyncThread<impl IntoLuaMulti, R>
    where
        R: FromLuaMulti,
    {
        AsyncThread {
            thread: self,
            init_args: Some(args),
            ret: PhantomData,
            recycle: false,
        }
    }

Instead, this should probably use a second type parameter of A in into_async to allow for rust to correctly infer the type using the passed value for args like so:

    pub fn into_async<A, R>(self, args: A) -> AsyncThread<A, R>
    where
        A: IntoLuaMulti
        R: FromLuaMulti,
    {
        AsyncThread {
            thread: self,
            init_args: Some(args),
            ret: PhantomData,
            recycle: false,
        }
    }
@khvzak
Copy link
Member

khvzak commented Jan 6, 2025

Right now, Thread::into_async is not really usable

Could you provide more details/example please?

@khvzak
Copy link
Member

khvzak commented Jan 7, 2025

I probably better remove A completely and instead push args to the thread.
This may also solve #500

@cheesycod
Copy link
Author

cheesycod commented Jan 7, 2025

Right now, Thread::into_async is not really usable

Could you provide more details/example please?

The issue right now is that the returned AsyncThread is a AsyncThread<impl IntoLuaMulti, R> which cannot be (easily) put in a struct. So the below is not possible:

pub struct A { a: AsyncThread<LuaMultiValue, LuaMultiValue> }

A {a: thread.into_async::<LuaMultiValue>(args) } // Assume args is a LuaMultiValue for sake of brevity

Due to the lack of genericizing on args, rust does not realise that the A of AsyncThread is actually a LuaMultiValue (from args) leading to a value that cannot be (easily) put into a struct and can basically only exist as a variable.

I probably better remove A completely and instead push args to the thread.

This may also solve #500

This is probably the better long-term option yeah.

khvzak added a commit that referenced this issue Jan 11, 2025
Push arguments in `Thread::into_async()` to the thread during the call instead of first poll.
The pushed arguments will be automatically used on resume.
Fixes #508 and relates to #500.
@khvzak
Copy link
Member

khvzak commented Jan 11, 2025

Unfortunately both options are a breaking change.

I committed to the dev branch the new approach with pushing args to the thread during into_async and removed A generic.

@cheesycod
Copy link
Author

Unfortunately both options are a breaking change.

I committed to the dev branch the new approach with pushing args to the thread during into_async and removed A generic.

This fixes the issue (still need to figure out the task lib stuff but the mlua AsyncThread works fine in structs now + can be reused at least for a perf improvement)

@cheesycod
Copy link
Author

@khvzak In addition to this, can a yield method (lua_yield in C API) be added to mlua. Right now, this forces me to use a small luau wrapper script to just do coroutine.yield()

@cheesycod
Copy link
Author

cheesycod commented Jan 20, 2025

@khvzak Also, can lua_resumeerror be added to mlua somehow? This is a Luau specific C API for resuming a thread with the error being the top value on stack

@khvzak
Copy link
Member

khvzak commented Jan 21, 2025

can a yield method (lua_yield in C API) be added to mlua.

It will not work since you cannot yield across C/Rust-call boundary.

@cheesycod
Copy link
Author

can a yield method (lua_yield in C API) be added to mlua.

It will not work since you cannot yield across C/Rust-call boundary.

Btw, do you mind reviewing #513

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants