Migrator is not Send

[Palmik]

Migrator is not Send, so running it asynchronously from futures that need to be Send (e.g. for async_trait) is not possible.

[abonander]

@Palmik can you please give the full compilation error? There's nothing in Migrator that should make it !Send so something else must be going on.

[Palmik]

@abonander Here's a minimal repro:

async fn test() {
    let pool = sqlx::PgPool::connect("foobar").await.unwrap();
    let migrator = sqlx::migrate::Migrator::new(std::path::Path::new("foo"))
        .await
        .unwrap();
    migrator.run(&pool).await.unwrap()
}

fn test_wrap() -> impl std::future::Future<Output = ()> + Send {
    test()
}

Error:

error: implementation of `sqlx::Acquire` is not general enough
  --> bin/xxx/src/main.rs:9:19
   |
9  |   fn test_wrap() -> impl std::future::Future<Output = ()> + Send {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `sqlx::Acquire` is not general enough
   |
  ::: /home/palmik/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/sqlx-core-0.4.2/src/acquire.rs:8:1
   |
8  | / pub trait Acquire<'c> {
9  | |     type Database: Database;
10 | |
11 | |     type Connection: Deref<Target = <Self::Database as Database>::Connection> + DerefMut;
...  |
15 | |     fn begin(self) -> BoxFuture<'c, Result<Transaction<'c, Self::Database>, Error>>;
16 | | }
   | |_- trait `sqlx::Acquire` defined here
   |
   = note: `&Pool<sqlx::Postgres>` must implement `sqlx::Acquire<'0>`, for any lifetime `'0`...
   = note: ...but `&Pool<sqlx::Postgres>` actually implements `sqlx::Acquire<'1>`, for some specific lifetime `'1`

error: aborting due to previous error

If you remove the Send constraint on test_wrap, it compiles. In practice this is a problem when using async_trait (unless I use it with ?Send).

But I looked into Migrator and also could not find why it would not be Send.

[abonander]

I'm not entirely sure that this is actually triggered by the presence of the + Send bound.

I'm decently sure, though, that the problem is here:

sqlx/sqlx-core/src/migrate/migrator.rs

Lines 46 to 50 in 8e4d061

	pub async fn run<'a, A>(&self, migrator: A) -> Result<(), MigrateError>
	where
	A: Acquire<'a>,
	<A::Connection as Deref>::Target: Migrate,
	{

The issue is, however, that we need to specify that A implements for<'a> Acquire<'a> but also that for<'a> <<<A as Acquire<'a>>::Connection> as Deref>::Target: Migrate and those kinds of lifetime bounds tend not to be reified well by the compiler.

A semi-fix might be to change this impl block:

sqlx/sqlx-core/src/acquire.rs

Line 18 in 8e4d061

impl<DB: Database> Acquire<'static> for &'_ Pool<DB> {

to be impl<'a, 'b, DB> Acquire<'a> for &'b Pool<DB> where DB: Database {} but I'm not entirely sure what else this will break.

We need a label that's like "HRTB-screwiness" that we can point the Rust compiler team at and be like "figure this out, will you".

cc @mehcode

[Palmik]

Right, I am not sure about the root cause, but without + Send it compiles. Thank you for investigating.

For those that stumble on this issue looking for a workaround on running migrations e.g. in their async_trait methods (which is how I stumbled on this), the following works fine on tokio's multi-threaded runtime:

futures::executor::block_on(async {
  sqlx::migrate!().run(pool).await
})

Sync it's something one would likely only run at startup, it seems like a reasonable solution.

[jplatte]

Based on my findings in #1015, I think I understand what's causing this.

pub async fn run<'a, A>(&self, migrator: A) -> Result<(), MigrateError> 
where 
    A: Acquire<'a>, 
    <A::Connection as Deref>::Target: Migrate, 

returns a future with lifetime 'a when really its lifetime should be the same as that of A. I don't think there is a way to express this with async fn, this function probably has to be rewritten to be a regular fn that returns impl Future. There's also a compiler bug that has to be worked around, #1020 is for that. I think after that, the correct definition is

pub fn run<'a, 'c, A>(&self, migrator: A) -> impl Future<Output = Result<(), MigrateError> + Send + 'a
where
    A: Acquire<'c> + 'a, 
    <A::Connection as Deref>::Target: Migrate,

[jplatte]

Haven't been able to figure out how to rewrite this to make it work in the same way as run_query in #1015. I'm not even entirely sure it's possible. In any case making the Acquire impl for Pool more general seems like a very good idea and I'd be surprised if it would break anything (I believe it would even be backwards-compatible). I've added that to #1020.