-
Notifications
You must be signed in to change notification settings - Fork 669
Add an executor::Wait trait
#487
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
Conversation
|
As a reference, here is the tokio-core snippet from #436 (comment). extern crate tokio_core;
fn main() {
tokio_core::run(|handle| {
let future = ...;
let result = future.wait();
println!("{:?}", result);
});
} |
carllerche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good. I added a comment regarding the panic guarantees of Wait::wait.
| /// called recursively due to multiple invocations of `Future::wait` on the | ||
| /// stack. In other words its expected for executors to handle | ||
| /// waits-in-waits correctly. | ||
| fn wait(&self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this function should be permitted to panic if the current executor does not allow for blocking the current thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right yes good point.
src/task_impl/std/mod.rs
Outdated
| /// It is expected that this method will not panic. This method may be | ||
| /// called recursively due to multiple invocations of `Future::wait` on the | ||
| /// stack. In other words its expected for executors to handle | ||
| /// waits-in-waits correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that in case of recursion the inner wait invocation may block the outer one until the inner wait finishes? This does not sound like "shortly thereafter".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanriol you are very correct. If nested calls to wait are done, then there are no guarantees as to when wait returns. In fact, there could be cases where wait never returns (because someone called wait on tcp_listener.incoming().for_each(...).
This is definitely a significant hazard and great care should be taken with this feature.
|
Ok, I've pushed an updated which I believe addresses comments. I'd like to integrate this into tokio-core first before landing. |
|
I think it would be wise to first agree how tokio-core should use this. I have significant reservations about supporting recursive calls to wait w/ tokio-core as there is no way to guarantee that futures ever complete. This isn't just a hypothetical concern as historically, other "future" platforms built on top of Java's Fork/Join pool did exactly this and my recollection is that it did not end well. IIRC, this was attempted w/ Akka and other less known libs and has since been removed. Maybe someone who is more familiar w/ these libs could provide further insight. |
This commit adds a new trait, `Wait`, to serve as the implementation for functions like `Future::wait`. The intention for this trait is for executors to be able to customize the blocking behavior of futures. Closes #360
4fa4ed4 to
2c6d0fa
Compare
|
I believe this this is now a "won't fix" with the direction being taken in #645. Please re-open if I am wrong. |
This commit adds a new trait,
Wait, to serve as the implementation forfunctions like
Future::wait. The intention for this trait is for executors tobe able to customize the blocking behavior of futures.
Closes #360