minkan-chat / server

This repository keeps the backend implementation for the Minkan end-to-end encrypted messenger.
GNU Affero General Public License v3.0
16 stars 1 forks source link

A compile error with Moka v0.5.2 or newer #12

Closed tatsuya6502 closed 2 years ago

tatsuya6502 commented 2 years ago

Hi. I am the author of Moka cache crate. I found minkan-chat server is using Moka. Thank you for choosing it!

I noticed that minkan-chat server no longer compiles if Moka v0.5.2 or newer is used. Let me explain why and how to solve it.

Why?

When compiling minkan-chat server with a recent version of Moka, the following error will occur:

error[E0759]: `ctx` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement
   --> src/models/graphql/mutations.rs:467:63
    |
467 |     pub(super) async fn get_token_expiry(user_id: uuid::Uuid, ctx: &Context<'_>) -> i64 {
    |                                                               ^^^  ------------ this data with an anonymous lifetime `'_`...
    |                                                               |
    |                                                               ...is captured here...
...
472 |             .get_or_insert_with(user_id, async {
    |              ------------------ ...and is required to live as long as `'static` here

This is because the signature of get_or_insert_with method was changed in v0.5.2.

v0.5.1 (doc)

pub async fn get_or_insert_with(&self, key: K, init: impl Future<Output = V>) -> V

v0.5.2 (doc)

pub async fn get_or_insert_with<F>(&self, key: K, init: F) -> V where
    F: Future<Output = V> + Send + 'static, 

As you can see, Send and 'static bounds were added to the generic parameter F of the init future. I had to do this to avoid undefined behaviors in some cases. I wrote a couple of examples here: https://github.com/moka-rs/moka/issues/31

This is the relevant code from minkan-chat server:

    pub(super) async fn get_token_expiry(user_id: uuid::Uuid, ctx: &Context<'_>) -> i64 {
        let cache = ctx.data::<Cache<uuid::Uuid, i64>>().unwrap();
        cache
            .get_or_insert_with(user_id, async move {
                let db = ctx.data::<PgPool>().unwrap(); // &Pool<Postgres>
                debug!("Getting token expiry for {}", &user_id);
                let r = sqlx::query!(
                    r#"
            SELECT token_expiry FROM users WHERE user_id = $1
            "#,
                    user_id
                )
                .fetch_one(db)
                .await;

                r.expect("The database didn't return a token_expiry")
                    .token_expiry
                    .timestamp()
            })
            .await
    }

The compile error occurs because ctx: &Context<'_>, which has not-static lifetime, is captured in the async block for get_or_insert_with. Moka v0.5.2 or newer no longer accept it as the async block will be executed later by different executor threads in the asynchronous runtime, and by the time, the reference (Context<'_>) may outlive the referent (the actual value).

How to solve the error

To solve the error, we need to change the async block not to capture Context<'_>, but to have the ownership of a db (Pool<Postgres>). Here is a solution. I added + and - signs to the beginning of lines to indicate the added/removed ones.

     pub(super) async fn get_token_expiry(user_id: uuid::Uuid, ctx: &Context<'_>) -> i64 {
         let cache = ctx.data::<Cache<uuid::Uuid, i64>>().unwrap();
+        // Create a clone of `Pool<Postgres>`.
+        let db = ctx.data::<PgPool>().unwrap().clone();
         cache
+            // Give the ownership of `db` to the async block by using
+            // the `move` keyword.
+            .get_or_insert_with(user_id, async move {
-
                 debug!("Getting token expiry for {}", &user_id);
                 let r = sqlx::query!(
                     r#"
             SELECT token_expiry FROM users WHERE user_id = $1
             "#,
                     user_id
                 )
+                // Pass a reference of `db`.
+                .fetch_one(&db)
                 .await;

                 r.expect("The database didn't return a token_expiry")
                     .token_expiry
                     .timestamp()
             })
             .await
     }

The downside of this solution is that get_token_expiry will always clone a Pool<Postgres> no matter if the cache has the token expiry or not. I think cloning a Pool<T> will be an inexpensive operation as it will only increment/decrement the reference count of Arc. So I hope this is negligible.

Please let me know if you have some questions.

Erik1000 commented 2 years ago

Hi! Big thanks for this issue. It means a lot to me that you care that much about your project! This is truly unique. I'm currently working on a refactor in the refactor branch because the code in main currently kinda sucks (as you've probably seen while writing the fix). I'm implementing a fix on that branch and will eventually merge it into main.

Again, thank you!