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

main_exeuctor from smol-macros #134

Closed
wants to merge 4 commits into from

Conversation

brandonros
Copy link

@notgull
Copy link
Member

notgull commented Oct 27, 2024

I feel like this is an XY problem. What is the use case here? MainExecutor is an awkward interface for a public trait, which is why it's not public in smol-macros.

@brandonros
Copy link
Author

It lets me do this:

fn main() -> Result<(), Box<dyn Error + Send + Sync>> {
    Arc::<Executor>::with_main(|ex| smol::block_on(async_main(ex)))
}

while boils down to this:

impl MainExecutor for Arc<Executor<'_>> {
    #[inline]
    fn with_main<T, F: FnOnce(&Self) -> T>(f: F) -> T {
        let ex = Arc::new(Executor::new());
        with_thread_pool(&ex, || f(&ex))
    }
}

which is mildly less awkward / more clear (to me at least)

We can close it if you think it doesn't gain anything. It felt weird to me that async-executor ships without a default that can handle multiple tasks on different threads or whatever but only smol-macros did with it tucked away/private.

@notgull
Copy link
Member

notgull commented Dec 3, 2024

Sorry for the delay. I guess my issue is that the MainExecutor trait is un-ergonomic and wasn't meant to be used outside of smol-macros. I can't really see a case where it excels compared to the macro. When I see this code:

fn main() -> Result<(), Box<dyn Error + Send + Sync>> {
    Arc::<Executor>::with_main(|ex| smol::block_on(async_main(ex)))
}

I don't see any advantages when compared to this code:

smol_macros::main! {
    fn main(ex: &Arc<Executor>) -> Result<(), Box<dyn Error + Send + Sync>> {
        async_main(ex);
    }
}

In fact, it seems less clear to me what the intent is.

@brandonros
Copy link
Author

I don't see any advantages when compared to this code:

It's a 2nd package you have to pull / it's awkward.

MainExecutor trait is un-ergonomic

What can be done to make it ergonomic? It's a little strange to pull a package smol that comes with a whole bunch of stuff, but the code you need to actually use it is off to the side with a private implementation. It's a core part of the functionality (thread pool) and it's like a back-seat citizen that is not exposed to documentation.

@notgull
Copy link
Member

notgull commented Dec 3, 2024

It's a 2nd package you have to pull / it's awkward.

It doesn't fit well in async-executor either; right now the crate is very unopinionated when it comes to creating the threadpool, and this would make it opinionated.

It's a little strange to pull a package smol that comes with a whole bunch of stuff, but the code you need to actually use it is off to the side with a private implementation.

One advantage of having it like this is that we're not locked into this particular implementation of a thread pool. If we want to change to a more efficient implementation of a thread pool in the future, we can do this without violating any of smol-macros's semver requirements.

It's a core part of the functionality (thread pool) and it's like a back-seat citizen that is not exposed to documentation.

I disagree. smol was designed from the ground up to be threadpool-agnostic. smol-macros only provides one for easy scaffolding. If you're at the point where you're having opinions about the thread pool, it's probably time to move off of smol-macros and create a thread pool with std::thread::scope.

cc @smol-rs/admins Design/usability problem

@brandonros brandonros closed this Dec 3, 2024
@brandonros
Copy link
Author

Slightly relevant comment for 4 years ago: smol-rs/smol#202 (comment)

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

Successfully merging this pull request may close these issues.

2 participants