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 an unstable task::blocking function #251

Merged
merged 4 commits into from
Oct 9, 2019
Merged

Conversation

yoshuawuyts
Copy link
Contributor

@yoshuawuyts yoshuawuyts commented Sep 27, 2019

This add an unstable task::spawn_blocking function to unblock people (lol) waiting for #65. I'm not sure if this is the right API, but we've talked about adding spawn_local at some point in the future too, so it doesn't seem too wrong either.

Regardless, I think it's time to provide people with at least "unstable" access to this. Thanks!

Closes #219, closes #65

cc/ @zkat

Screenshot

Screenshot_2019-09-28 async_std task - Rust

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Sep 27, 2019
@yoshuawuyts yoshuawuyts requested a review from a user September 27, 2019 23:39
@yoshuawuyts yoshuawuyts mentioned this pull request Sep 27, 2019
@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Sep 28, 2019

I still think we should merge this, but I'm almost wondering if the mapping of blocking -> future should be part of the thread submodule instead.

There's something about blocking tasks that makes me feel like it should be part of that. Not sure tho; feel like we may need to talk it through sometime.

@skade
Copy link
Collaborator

skade commented Sep 29, 2019

I see the issue that this function will be effectively stable. The proliferation of reaching for an unstable flag to expose things people are asking for will either force people to choose the unstable flag all the time and not caring about it and taking the ability from us to iterate on the solution.

We should instead spend more time on figuring out what a stable blocking interface should be.

@dvc94ch
Copy link

dvc94ch commented Sep 29, 2019

Having to make some changes after running cargo update is better than having to maintain a fork, that is out of date. And isn't semver supposed to alleviate this? I don't understand all this commitment to find a solution that will be supported indefinitly.

@dvc94ch
Copy link

dvc94ch commented Sep 29, 2019

After some reflection I see the problem. Whenever a crate exports a trait the crate can only occur once in the dependency tree. Crates with public enums, structs with pub fields and functions that take a struct as an argument have a similar problem.

@zkat
Copy link

zkat commented Oct 6, 2019

Yes please! Is this moving forward? :D

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Oct 6, 2019

@skade We've been talking about this for months now, and we haven't made any tangible progress on an API design. Currently there's quite a few other things in-progress, and several other things marked as priority before we could get around to designing this. Where I estimate it'll probably be at least another month or two before this can be prioritized, which imo is too long.

I think the higher order bit would be to unblock people who have been waiting on this feature for a long time already, so they can move forward and build projects on top of async-std. We can provide detailed upgrade instructions if we decide to stabilize a different version of the API.

cc/ @stjepang I'd be curious for your input here also.

edit: Also I suspect we'll slowly be removing many of the existing "unstable" labels in the near future. It's mostly pending review of APIs, and ensuring we didn't miss anything in the signatures.

@ghost
Copy link

ghost commented Oct 7, 2019

I'm comfortable with stabilizing this function, provided we do two changes:

  1. Rename to something simpler, perhaps task::blocking().
  2. Return a regular task::JoinHandle<T> from the function.

We should probably make blocking tasks behave just like normal tasks spawned using task::spawn(), except task::blocking() would give a hint to the scheduler that this task might block. That means inside blocking tasks we should be able to use task-locals, task::current() and so on.

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Oct 8, 2019

@stjepang and I had a chat yesterday, and we're on the same page about what to do. I agree with what he's said in #251 (comment).

The only addition I had to what was said is that we should in turn clearly document the difference between task::block_on and task::blocking, as this could be confusing for people new to async-std.

@yoshuawuyts yoshuawuyts changed the title add an unstable task::spawn_blocking function add an unstable task::blocking function Oct 8, 2019
Signed-off-by: Yoshua Wuyts <[email protected]>
@yoshuawuyts
Copy link
Contributor Author

Alright, this PR now exposes task::blocking, and implements #219 as requested. This should be good to review now!

@yoshuawuyts yoshuawuyts force-pushed the blocking-unstable branch 3 times, most recently from 4038938 to 647aab8 Compare October 8, 2019 12:54
Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
@yoshuawuyts yoshuawuyts mentioned this pull request Oct 8, 2019
@yoshuawuyts
Copy link
Contributor Author

I've addressed all feedback, and things are compiling + tests passing so I think this should be good to merge!

@yoshuawuyts yoshuawuyts merged commit 9ab7b1a into master Oct 9, 2019
@yoshuawuyts yoshuawuyts deleted the blocking-unstable branch October 9, 2019 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up task::Tag in blocking tasks Make blocking public?
4 participants