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

rt: add Handle::block_on #3549

Closed
wants to merge 1 commit into from
Closed

rt: add Handle::block_on #3549

wants to merge 1 commit into from

Conversation

carllerche
Copy link
Member

Add runtime::Handle::block_on. The function enters the runtime context and blocks the current thread while the future executes. This strategy does not do anything special with the current_thread runtime. If there is no thread "driving" the runtime, then neither spawned tasks nor I/O resources will not make progress. To avoid this, it is recommended to use the multi-threaded scheduler (perhaps w/ core-threads set to 1).

This is a draft PR to demonstrate the direction. Before merging, the PR would need API documentation and tests.

Refs: #3097
Fixes #2965, #3096

cc @stuhood, @Keruspe

/// TODO: write docs if this is a good direction
pub fn block_on<F: Future>(&self, future: F) -> F::Output {
// Enter the **runtime** context. This configures spawning, the current I/O driver, ...
let _rt_enter = self.enter();
Copy link
Member

Choose a reason for hiding this comment

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

can enter check if the runtime is still alive?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you opened a different issue, but that is someone of an orthogonal concern :)

let mut _blocking_enter = crate::runtime::enter(true);

// Block on the future
_blocking_enter.block_on(future).expect("failed to park thread")
Copy link
Member

Choose a reason for hiding this comment

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

Then this should be a block on that runs the task and can wake up on runtime shutdown. We likely wanna use the same notify trick?

Copy link
Member Author

Choose a reason for hiding this comment

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

The future isn't a task per se. When the runtime is shutdown, in theory all resources should transition to a "shutdown" state and in-flight operations canceled. This would result in this block_on waking up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the task is doing nothing? E.g. if I just call handle.block_on(std::future::pending()), then what?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I guess my gut is that calling Handle::block_on shouldn't necessarily force exit when the runtime shuts down. My gut reaction is that handle.block_on(std::future::pending()) would just run indefinitely.

@carllerche carllerche requested a review from a team February 23, 2021 21:01
Copy link

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

An infallible API like this certainly works for us if it is safe to implement this way. But if it is necessary to be fallible (returning Result<F::Output, TheRuntimeIsGone>) as @Darksonn has suggested due to Handle being a weak reference, that's fine with us too.

@Darksonn
Copy link
Contributor

There has been some concern about the behavior of Handle::block_on when the runtime shuts down. In this post, I outline my proposal for handling that:

  1. The call to block_on should continue to poll the provided future even if the runtime shuts down.
  2. Any IO resources or timers used inside the block_on should immediately report an error or panic.
  3. If the block_on is running executor-independent code, it should continue to run unaffected by the shutdown.

There was some concern regarding hanging futures. I believe the above proposal avoids any hanging tasks on runtime shutdown, but if there is some reason it cannot be implemented to avoid hangs, we will need some other solution.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 1, 2021

Should this be closed in favor of #3569 ?

@carllerche
Copy link
Member Author

yes.

@carllerche carllerche closed this Mar 9, 2021
@Darksonn Darksonn deleted the rt-handle-block-on branch March 20, 2021 09:57
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.

Replacement for Handle::current
5 participants