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

Add support for effects with custom runtimes #3469

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LLBlumire
Copy link

Fixes #3468

This is almost certainly not the best way to do it, but I couldn't figure out the generic types so did a pinned box instead, but it demonstrates the functionality that is desired.

Copy link
Collaborator

@gbj gbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, overall! Couple small notes on docs, and a suggestion if you do want to use generics rather requiring the function to accept a pinned-box -- although you should probably make that decision based on how you're going to consume it! (i.e., if the current signature is easier to use then go with this one)

///
/// This spawns a task on the local thread using
/// [`spawn_local`](any_spawner::Executor::spawn_local). For an effect that can be spawned on
/// any thread, use [`new_sync`](Effect::new_sync).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs should probably be updated to

  1. Note that this allows you to pass in a runtime, and
  2. Update the new_sync x 2 here to new_sync_with_runtime

/// any thread, use [`new_sync`](Effect::new_sync).
pub fn new_with_runtime<T, M>(
mut fun: impl EffectFunction<T, M> + 'static,
pass_to_rt: impl FnOnce(Pin<Box<dyn Future<Output = ()> + 'static>>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest a name like spawn here rather than pass_to_rt as that's the phrase we tend to use for "spawn a task" elsewhere

I think the signature can be something like

    pub fn new_with_runtime<T, M, F, Fut>(
        mut fun: impl EffectFunction<T, M> + 'static,
        pass_to_rt: F
    ) -> Self 
    where 
        T: 'static,
        F: FnOnce(Fut) + 'static,
        Fut: Future<Output = ()>

if you want to avoid the Pin<Box<_>> here

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

Successfully merging this pull request may close these issues.

Provide executor to Effect at creation
2 participants