Upgrade to async-session v4

[sbihel]

Replaces #29

This PR is mostly for people who have encountered issues such as #32 and need a temporary fix. It seems to work fine in my case.

[maxcountryman]

Thanks for this. I'd love to merge the upstream changes asap so we can fix the session extractor but at the same time for that to be useful the other crates that use it will also need to be updated. I've also not had much luck contacting the maintainers recently.

[maxcountryman]

Another important point here: we should retire the session extractor pair and instead use the session directly.

[sbihel]

Thanks for this. I'd love to merge the upstream changes asap so we can fix the session extractor but at the same time for that to be useful the other crates that use it will also need to be updated. I've also not had much luck contacting the maintainers recently.

Yeah, it is unfortunate. But he's still active generally speaking so hopefully things will fall into place soon

Another important point here: we should retire the session extractor pair and instead use the session directly.

Ah, I was wondering about this. SessionHandle will still be needed right?

[maxcountryman]

SessionHandle will still be needed right?

My hope is that we can avoid the RWLock since that seems like a poor design decision on my part (although it's unclear what other options we have with the current implementation of async-session). Because the prior Clone implementation has been removed, we would hopefully instead be able to use Session directly.

[maxcountryman]

Previously, we used to insert the session directly. So perhaps we could simply go back to this and avoid the handle.

[sbihel]

I've had a go at getting rid of SessionHandle but I can't find a way of making it work.

I agree that the RwLock shouldn't be necessary, because the async_session::Session is loaded for each request, sequentially goes through the Layers, is modified by the user, and then the Session Service updates the store accordingly.

But extensions are not automatically propagated from Request to Response. So an Arc is necessary, and because of the mut requirement, we end up with SessionHandle again.

That being said, RwLock should probably be replaced with a Mutex, and the public type SessionHandle can be replaced with an alias for MutexGuard?

[maxcountryman]

Thanks for doing that. I agree with you assessment. I haven't looked closely at the Mutex semantics, but assuming it improves the ergonomics (i.e. reduces or eliminates deadlocking) then yes.

[maxcountryman]

I think something like this could work:

diff --git a/src/extractors.rs b/src/extractors.rs
index 9b9a547..85fe0ca 100644
--- a/src/extractors.rs
+++ b/src/extractors.rs
@@ -3,78 +3,46 @@
 use std::ops::{Deref, DerefMut};
 
 use axum::{async_trait, extract::FromRequestParts, http::request::Parts, Extension};
-use tokio::sync::{OwnedRwLockReadGuard, OwnedRwLockWriteGuard};
+use tokio::sync::OwnedMutexGuard;
 
 use crate::SessionHandle;
 
 /// An extractor which provides a readable session. Sessions may have many
 /// readers.
 #[derive(Debug)]
-pub struct ReadableSession {
-    session: OwnedRwLockReadGuard<async_session::Session>,
+pub struct Session {
+    session_guard: OwnedMutexGuard<async_session::Session>,
 }
 
-impl Deref for ReadableSession {
-    type Target = OwnedRwLockReadGuard<async_session::Session>;
+impl Deref for Session {
+    type Target = OwnedMutexGuard<async_session::Session>;
 
     fn deref(&self) -> &Self::Target {
-        &self.session
+        &self.session_guard
     }
 }
 
-#[async_trait]
-impl<S> FromRequestParts<S> for ReadableSession
-where
-    S: Send + Sync,
-{
-    type Rejection = std::convert::Infallible;
-
-    async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
-        let Extension(session_handle): Extension<SessionHandle> =
-            Extension::from_request_parts(parts, state)
-                .await
-                .expect("Session extension missing. Is the session layer installed?");
-        let session = session_handle.read_owned().await;
-
-        Ok(Self { session })
-    }
-}
-
-/// An extractor which provides a writable session. Sessions may have only one
-/// writer.
-#[derive(Debug)]
-pub struct WritableSession {
-    session: OwnedRwLockWriteGuard<async_session::Session>,
-}
-
-impl Deref for WritableSession {
-    type Target = OwnedRwLockWriteGuard<async_session::Session>;
-
-    fn deref(&self) -> &Self::Target {
-        &self.session
-    }
-}
-
-impl DerefMut for WritableSession {
+impl DerefMut for Session {
     fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.session
+        &mut self.session_guard
     }
 }
 
 #[async_trait]
-impl<S> FromRequestParts<S> for WritableSession
+impl<S> FromRequestParts<S> for Session
 where
-    S: Send + Sync,
+    S: Send + Sync + Clone,
 {
     type Rejection = std::convert::Infallible;
 
-    async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
-        let Extension(session_handle): Extension<SessionHandle> =
-            Extension::from_request_parts(parts, state)
-                .await
-                .expect("Session extension missing. Is the session layer installed?");
-        let session = session_handle.write_owned().await;
+    async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result<Self, Self::Rejection> {
+        use axum::RequestPartsExt;
+        let Extension(session) = parts
+            .extract::<Extension<SessionHandle>>()
+            .await
+            .expect("Session extension missing. Is the session layer installed?");
 
-        Ok(Self { session })
+        let session_guard = session.lock_owned().await;
+        Ok(Self { session_guard })
     }
 }
diff --git a/src/session.rs b/src/session.rs
index 0f632d9..6947bfb 100644
--- a/src/session.rs
+++ b/src/session.rs
@@ -20,7 +20,7 @@ use base64::{engine::general_purpose::STANDARD as BASE64, Engine};
 use futures::future::BoxFuture;
 use hmac::{Hmac, Mac};
 use sha2::{digest::generic_array::GenericArray, Sha256};
-use tokio::sync::RwLock;
+use tokio::sync::Mutex;
 use tower::{Layer, Service};
 
 const BASE64_DIGEST_LEN: usize = 44;
@@ -34,7 +34,7 @@ const BASE64_DIGEST_LEN: usize = 44;
 /// than using the handle directly. A notable exception is when using this
 /// library as a generic Tower middleware: such use cases will consume the
 /// handle directly.
-pub type SessionHandle = Arc<RwLock<async_session::Session>>;
+pub type SessionHandle = Arc<Mutex<async_session::Session>>;
 
 /// Controls how the session data is persisted and created.
 #[derive(Clone)]
@@ -189,7 +189,7 @@ where
             None => None,
         };
 
-        Arc::new(RwLock::new(
+        Arc::new(Mutex::new(
             session
                 .and_then(async_session::Session::validate)
                 .unwrap_or_default(),
@@ -331,22 +331,21 @@ where
         let mut inner = std::mem::replace(&mut self.inner, inner);
         Box::pin(async move {
             let session_handle = session_layer.load_or_create(cookie_value.clone()).await;
+            let mut session = session_handle.lock().await;
 
-            let mut session = session_handle.write().await;
             if let Some(ttl) = session_layer.session_ttl {
                 (*session).expire_in(ttl);
             }
             drop(session);
 
             request.extensions_mut().insert(session_handle.clone());
+
             let mut response = inner.call(request).await?;
 
-            let session = session_handle.read().await;
+            let mut session = session_handle.lock().await;
             let (session_is_destroyed, session_data_changed) =
                 (session.is_destroyed(), session.data_changed());
-            drop(session);
 
-            let mut session = session_handle.write().await;
             if session_is_destroyed {
                 if let Err(e) = session_layer.store.destroy_session(&mut session).await {
                     tracing::error!("Failed to destroy session: {:?}", e);

[maxcountryman]

There's a build failing which I think is related to the sqlx example. I'm maintaining a separate fork of that repo, so if we'd like to swap that in that should fix the build.

[maxcountryman]

This is I think completely orthogonal to Mutex, but I did want to point out that there is the potential for data races here. In situations where routes read from a session and then write to that session, the read value can become stale between reading and writing. Because async_session::Session doesn't have a compare_and_swap mechanism, I think this might be true in any multi-threaded context (at least assuming there aren't protections in place on top of async_session::Session's API).

[sbihel]

This is I think completely orthogonal to Mutex, but I did want to point out that there is the potential for data races here. In situations where routes read from a session and then write to that session, the read value can become stale between reading and writing. Because async_session::Session doesn't have a compare_and_swap mechanism, I think this might be true in any multi-threaded context (at least assuming there aren't protections in place on top of async_session::Session's API).

That's fair, but I think it's a session store concern, be it in the implementation or in the API? But I don't know how worrying it is in the context of browser sessions.

[maxcountryman]

but I think it's a session store concern

In async_session there's a layer of indirection between the session and its storage (i.e. the session store) so even if a store provides atomic operations, this would need to be addressed via this layer of indirection. (Incidentally, the session's data field is behind RwLock.) What I think this means is clients would need a compare_and_swap method to avoid these kinds of data races.

It's probably okay to punt that responsibility elsewhere. That said, I think we should at least document it as a foot gun even if most applications may not care about this. (I also wonder if we can work around this by plucking out the SessionHandle?)

[maxcountryman]

Hi folks, I started a discussion related to the direction we should take axum-sessions going forward and implemented a possible replacement in the form of tower-sessions.

The goal with this is to improve the axum-sessions interface, address as many of the pain points as we practically can, and ensure we aren't overly reliant on core functionality that's outside the crate.

There's a couple of folks who would like the session concept to be replaced with custom types, but I'm skeptical that's a good fit for a crate that's intended to evolve and replace this crate.

However, I would love your feedback more generally about tower-sessions.

[headironc]

When will the bug fix release be available

[maxcountryman]

@headironc its difficult to say precisely since this is dependent on async-session merging and releasing a PR that's been open for some time.

For now, you can use this directly via git.

At the same time, I've also started working on tower-sessions, which could circumvent these issues altogether.

Feedback appreciated.

[headironc]

@headironc its difficult to say precisely since this is dependent on async-session merging and releasing a PR that's been open for some time.

For now, you can use this directly via git.

At the same time, I've also started working on tower-sessions, which could circumvent these issues altogether.

Feedback appreciated.

oh, thanks much

[headironc]

I still have this problem

This is dependencies, i use this Replace RwLock with Mutex

[dependencies]
axum-sessions = { git = "https://github.com/sbihel/axum-sessions", rev = "079da64729ddaed5d238bcec1abbcfb83a0f6411" }

This is my middleware

const URI: [&str; 3] = [
    "/api/v1/users/register",
    "/api/v1/users/login",
    "/api/v1/users/logout",
];

pub async fn auth<T>(
    session: Session,
    State(state): State<AppState>,
    mut req: Request<T>,
    next: Next<T>,
) -> Result<impl IntoResponse, AppError> {
    event!(Level::INFO, "auth middleware");

    let uri = req.uri().to_string();

    return Ok(next.run(req).await);

    // if URI.contains(&uri.as_str()) {
    //     return Ok(next.run(req).await);
    // }

    // let user_id = if let Some(id) = session.get::<ObjectId>("user") {
    //     id
    // } else {
    //     event!(Level::INFO, "no user_id found in session");

    //     return Err(Unauthorized);
    // };

    // let persist = state.persistence();

    // if let Some(current_user) = User::find_by_id(persist, user_id).await? {
    //     event!(Level::INFO, username = ?current_user.username(), user_id = ?current_user.id().to_hex(), "current_user");

    //     req.extensions_mut().insert(current_user);

    //     Ok(next.run(req).await)
    // } else {
    //     Err(Unauthorized)
    // }
}

This is my handler.

pub async fn logout(mut session: Session) -> Result<impl IntoResponse, AppError> {
    session.destroy();

    Ok(StatusCode::NO_CONTENT)
}

Still block request

This worked

const URI: [&str; 3] = [
    "/api/v1/users/register",
    "/api/v1/users/login",
    "/api/v1/users/logout",
];

pub async fn auth<T>(
    // session: Session,
    State(state): State<AppState>,
    mut req: Request<T>,
    next: Next<T>,
) -> Result<impl IntoResponse, AppError> {
    event!(Level::INFO, "auth middleware");

    let uri = req.uri().to_string();

    return Ok(next.run(req).await);

    // if URI.contains(&uri.as_str()) {
    //     return Ok(next.run(req).await);
    // }

    // let user_id = if let Some(id) = session.get::<ObjectId>("user") {
    //     id
    // } else {
    //     event!(Level::INFO, "no user_id found in session");

    //     return Err(Unauthorized);
    // };

    // let persist = state.persistence();

    // if let Some(current_user) = User::find_by_id(persist, user_id).await? {
    //     event!(Level::INFO, username = ?current_user.username(), user_id = ?current_user.id().to_hex(), "current_user");

    //     req.extensions_mut().insert(current_user);

    //     Ok(next.run(req).await)
    // } else {
    //     Err(Unauthorized)
    // }
}

[maxcountryman]

Sorry you're running into this. Something is holding the lock, which unfortunately is a clear drawback of how axum-sessions is designed today. Whether we use Mutex or RwLock, in either case we're using a lock so that doesn't change the fundamental issue with locking as we are.

For that reason, I took a different approach with tower-sessions: we still need a Mutex or RwLock, because without this we can't have shared mutable state. However, we can defer locking until we actually invoke some operation over the underlying hash map. This is how tower-sessions works. Deadlocks should be far less likely with this approach. (Altho there is still a lock: that said, this is how other tower middleware, like tower-cookies, work so I have some confidence it's a pattern that should work well in practice.)

I'm close to publishing an initial release of tower-sessions and if you'd like to try that with your use case it would be helpful to know this new approach does address this problem.

[headironc]

Sorry you're running into this. Something is holding the lock, which unfortunately is a clear drawback of how axum-sessions is designed today. Whether we use Mutex or RwLock, in either case we're using a lock so that doesn't change the fundamental issue with locking as we are.

For that reason, I took a different approach with tower-sessions: we still need a Mutex or RwLock, because without this we can't have shared mutable state. However, we can defer locking until we actually invoke some operation over the underlying hash map. This is how tower-sessions works. Deadlocks should be far less likely with this approach. (Altho there is still a lock: that said, this is how other tower middleware, like tower-cookies, work so I have some confidence it's a pattern that should work well in practice.)

I'm close to publishing an initial release of tower-sessions and if you'd like to try that with your use case it would be helpful to know this new approach does address this problem.

I'm looking forward to the new library tower-sessions

[maxcountryman]

@headironc I've released v0.1.0 if you'd like to try.