Add support for std::future and async/await

[jonhoo]

Still a work in progress.

This is a very direct port of the existing codebase. I'm avoiding rearchitecting as much as I can, and instead doing a mostly mechanical transformation. Seems to be going okay so far, but I'm not quite done yet (need tokio-rs/tokio#1537 for example). There are definitely a bunch of places the codebase could be improved (in particular, a bunch of Boxes that could be removed), but at least this will get us off the ground.

Fixes #63.

[jonhoo]

Will also need someone with access to a Windows machine to port https://github.com/nikvolf/tokio-named-pipes. Shouldn't be too hard I think.

[jonhoo]

Currently seemingly stuck on rust-lang/rust#46415 (comment) -- the compiler runs out of stack when trying to compile the code, presumably due to a cyclic async fn somewhere.

[jonhoo]

I think in theory we could also improve some of the APIs a fair amount as part of this process. For example, we no longer really need to have all the futures return Self, since we can now take &mut self instead!

[blackbeam]

It's a lot of work, thanks!

Will also need someone with access to a Windows machine to port https://github.com/nikvolf/tokio-named-pipes. Shouldn't be too hard I think.

I've downloaded a fresh image from https://developer.microsoft.com/en-us/windows/downloads/virtual-machines a week ago, but I'm not sure on how soon i would be able to work on this. I need to close some education-related stuff during this weekend because of a deadline.

[jonhoo]

@blackbeam I actually reached out to @NikVolf (the maintainer) and got a reply saying they'd take a look!

[PvdBerg1998]

Thanks for your work! Looking forward to finally ditching futures 0.1 (this is my last dependency using it)!

[jonhoo]

Tests now pass on CircleCI (though they require nightly of course) 🎉 Windows won't work until we have tokio-named-pipes, so I think that's the blocker for now?

Also, @blackbeam, how would you feel about moving to Azure Pipelines using https://github.com/crate-ci/azure-pipelines ? It's pretty easy to set up, and would mean we don't have to have two configs that have tons of random command strings everywhere.

[PvdBerg1998]

@jonhoo I've looked into porting tokio-named-pipes but it seems the crate is redundant now, you can just use PollEvented directly I think. See https://github.com/tokio-rs/tokio/blob/f1f61a3b15d17767b12bfd8c0c5712db1b089b0b/tokio-net/src/util/poll_evented.rs#L356.

[jonhoo]

@PvdBerg1998 Ah interesting.. Trying that now.

[jonhoo]

Oh, that's awkward. What is it that fails? Presumably cargo should just compile both versions?

[PvdBerg1998]

Yeah I've been trying to patch it but it's being difficult for some reason. The error is:

versions that meet the requirements ^0.4.0-alpha.10 are: 0.4.0-alpha.10

all possible versions conflict with previously selected packages.

  previously selected package pin-project v0.4.0-alpha.7
    ... which is depended on by hyper v0.13.0-alpha.1`
    ... which is depended on by hyper-tls v0.4.0-alpha.1`
    ... which is depended on by projectname

failed to select a version for `pin-project` which could resolve this conflict

I tried patching it using pin-project = { version = "^0.4.0-alpha.10" } but patch for pin-project in https://github.com/rust-lang/crates.io-index points to the same source, but patches must point to different sources :(

[jonhoo]

@taiki-e Can you shed some light on the issue above?

[PvdBerg1998]

Ok so I just got cargo to panic 😅 . Edit: see rust-lang/cargo#7346

[taiki-e]

Can you shed some light on the issue above?

This is because pin-project is still alpha, so hyper is pinning that version. I think it needs to update version of pin-project in hyper and add the following block to Cargo.toml:

[patch.crates-io]
hyper = { git = "https://github.com/hyperium/hyper", version = "0.13.0-alpha.1" }

[PvdBerg1998]

@jonhoo I forked hyper and patched their dependency. It still doesn't compile because tokio master changed something. But: I'm getting a compilation error in async_tls:

error[E0277]: the trait bound `S: tokio_io::async_read::AsyncRead` is not satisfied
   --> C:\Users\pim\.cargo\git\checkouts\mysql_async-a98a8705a879e6ef\c859e17\src\io\async_tls.rs:112:26
    |
112 |         inner: connector.connect(domain, stream).await?,
    |                          ^^^^^^^ the trait `tokio_io::async_read::AsyncRead` is not implemented for `S`
    |
    = help: consider adding a `where S: tokio_io::async_read::AsyncRead` bound

error[E0277]: the trait bound `S: tokio_io::async_write::AsyncWrite` is not satisfied
   --> C:\Users\pim\.cargo\git\checkouts\mysql_async-a98a8705a879e6ef\c859e17\src\io\async_tls.rs:112:26
    |
112 |         inner: connector.connect(domain, stream).await?,
    |                          ^^^^^^^ the trait `tokio_io::async_write::AsyncWrite` is not implemented for `S`
    |
    = help: consider adding a `where S: tokio_io::async_write::AsyncWrite` bound

Edit: you have that bound there though... I'm confused
Edit 2: why is async_tls::TlsStream even required? It doesn't seem to add any functionality

[PvdBerg1998]

I forked the PR and removed the #[cfg()] so I can test it on Windows. Calling Pool::disconnect never returns for me.

[PvdBerg1998]

Other functionality seems to work 🎉

[jonhoo]

@blackbeam what do you think still remains for this?

[blackbeam]

@jonhoo, patch section should be removed from Cargo.toml. But at the moment this won't work, because tokio-rs/tokio#1537 not yet published.

[pimeys]

How hard it would be to make all the methods taking self by value to make them to be by reference? Right now abstracting mysql to a more generic database crate is not that easy due to other crates just using references. At least tokio-postgres uses an approach where you have a Client and Connection which communicating through a channel. In addition to a cleaner interface, this approach also allows us to do stuff like rolling back a transaction in Drop if not commited yet.

[jonhoo]

@pimeys so, I took a quick look at that when I did this PR, and while in theory it shouldn't be too bad, in practice a tonne of the code relies on being able to move in and out of things. This is (as you probably guessed) because much of the state keeping for each connection is done inside the Connection instance itself, rather than in some spawned state machine that is accessed through a channel. This has upsides (you don't have to own arguments to execute) and downsides (Connection can't easily be cloned and all receivers take self). I think changing this would be a larger refactoring.

[blackbeam]

@pimeys, that's interesting. Looks like major change to the current architecture. I'm not sure, however, about pros of this approach except API convenience.

[jonhoo]

@blackbeam It does make the API much nicer to use, because you no longer need to "chain along" Self by doing things like:

let result = c.query("...").await?;
let (c, rows) = result.collect_and_drop().await?;
let c = c.drop_query("...").await?;

instead you can just write code without moving c:

let rows = c.query("...").await?.collect_and_drop().await?;
c.drop_query("...").await?;

[blackbeam]

@jonhoo, agree with that. I believe it worth, at least, to try to implement it to measure the consequences.

[pimeys]

We here at Prisma are abstracting the databases (psql, mysql, sqlite) under our own connector crate, and right now with the sync crates the generic connector is very easy to do. Not that much when we switch to async execution, due to mysql being completely different than the others.

I've been reading the tokio-postgres code a lot and they seem to have a nice way of making it to work:

https://github.com/sfackler/rust-postgres/blob/std-futures/tokio-postgres/src/client.rs#L176

What makes their way even sweeter the way the transactions can be rolled back in Drop:

https://github.com/sfackler/rust-postgres/blob/std-futures/tokio-postgres/src/transaction.rs#L27

Now your transaction can work like the sync counterpart. If you error out, RAII takes care of the rollback.

[blackbeam]

@pimeys, btw, mysql_async transaction must rollback if not committed (at least if tokio runtime is alive). Please file an issue if you experienced the opposite.

[blackbeam]

Published as v0.21.0-alpha.1

[blackbeam]

@jonhoo, @pimeys,

New non-consuming API is available in v0.24.0-alpha. Could you, please, test it? (api docs)

[tomhoule]

I haven't tried very hard, but trying to use 0.24.0 alpha in quaint (prisma/quaint#117), we get a compile error from one of the dependecies:

    Checking time v0.2.8
error: expected an item keyword
   --> /home/tom/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.2.8/src/utc_offset.rs:366:13
    |
366 |             let tm = timestamp_to_tm(datetime.timestamp())?;
    |             ^^^

error: aborting due to previous error

(it's one of mysql_common's dependencies).

[blackbeam]

@tomhoule, it looks like this issue. I believe cargo update should help.