-
Notifications
You must be signed in to change notification settings - Fork 981
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
Update to tokio 0.2 and futures 0.3 #1448
Conversation
39f35c4
to
2b237ca
Compare
00a9a48
to
75d0dd0
Compare
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.
Mostly suggestions which I leave to your judgment, but there are a couple blocking issues here that need be addressed, particularly tests not running and futures not being polled.
@That3Percent Thank you for the review, I believe I've addressed all comments. |
There's a |
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.
Nice. LGTM
For tokio 0.2 compatibility
18ae057
to
fee5945
Compare
@Jannis You've caught me, I squashed that and rebased. After tests pass, I'll merge. |
I'm doing a review pass myself. Can you wait for that? I'll make it quick. 😄 |
@Jannis np at all, thanks for also reviewing. |
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.
Looking good! I merely have a few questions.
@Jannis I've addressed your review. |
This brings into the future of futures by upgrading to tokio 0.2, futures 0.3, hyper 0.13 and various other dependency upgrades. This not only allows us to use async/.await but also pushes us towards that by having updated dependencies. Some parts of the code were updated to use
async/.await
to serve as examples. This is working fine as far as I have tested it.Using std::future and async
There's now a
Future
trait that lives in std, though helpers are still in the futures crate. The major difference from the one in futures 0.1 is that it has a singleOutput
associated type, instead ofItem
andError
(same for streams). It is easy to convert back and forth between a std future that hasOutput = Result<_, _>
and the old futures by calling.compat()
.The
async
syntax can be thought of as a future literal, and works on fns and blocks (but not closures, yet). To useasync
in trait fns the async-trait crate is required, I haven't tried that yet. In blocks you usually wantasync move
. Inside an async fn or block, you can use.await
on futures as it were the oldand_then
, but more flexible. Using future combinators is still useful, so checkout the docs forFutureExt
andTryFutureExt
to catch up on them. There is no new syntax for streams, yet. Sometimes the compiler will complain aboutUnpin
, I haven't totally understood the pinning stuff but wrapping the future in aBox::pin
usually fixes that.Spawning tasks and panic handling
Usually we want a tokio task panic to abort the process so that the node has a chance to recover by restarting. Except in some situations such as in the runtime or in queries. So instead of using
tokio::spawn
directly, we should choose between:graph::spawn
which runs on the default threadpool and aborts on panic.graph::spawn_blocking
which is run on the blocking threadpool and aborts on panic.graph::spawn_blocking_allow_panic
which is run on the blocking threadpool and allows panics. A panic will result in an error in theJoinHandle
.Note that all of these now return a
JoinHandle
for joining the task, which can be very useful.Resolves #937, since we're now careful to use the blocking thread pool when called for, and we're also back to the default thread pool size.
Resolves #805, panics in query processing are no longer fatal.