From e77ed16f4928b4c8608a7678d847f753fc65c9ae Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 11 Dec 2021 16:23:22 +0000 Subject: [PATCH] Do not create a session if the session state is empty (#207) --- actix-redis/CHANGES.md | 2 + actix-redis/src/session.rs | 96 +++++++++++++++----------------------- 2 files changed, 39 insertions(+), 59 deletions(-) diff --git a/actix-redis/CHANGES.md b/actix-redis/CHANGES.md index c41b64b1a..de416f0b1 100644 --- a/actix-redis/CHANGES.md +++ b/actix-redis/CHANGES.md @@ -1,8 +1,10 @@ # Changes ## Unreleased - 2021-xx-xx +* A session will be created in Redis if and only if there is some data inside the session state. This reduces the performance impact of `RedisSession` on routes that do not leverage sessions. [#207] * Update `actix-web` dependency to `4.0.0.beta-14`. [#209] +[#207]: https://github.com/actix/actix-extras/pull/207 [#209]: https://github.com/actix/actix-extras/pull/209 diff --git a/actix-redis/src/session.rs b/actix-redis/src/session.rs index 16b7db0d1..282993cf2 100644 --- a/actix-redis/src/session.rs +++ b/actix-redis/src/session.rs @@ -170,13 +170,10 @@ where let mut res = srv.call(req).await?; match Session::get_changes(&mut res) { - (SessionStatus::Unchanged, state) => { - if value.is_none() { - // implies the session is new - inner.update(res, state, value).await - } else { - Ok(res) - } + (SessionStatus::Unchanged, _) => { + // If the session already exists, we don't need to update the state stored in Redis + // If the session is new, creating an empty session in Redis is unnecessary overhead + Ok(res) } (SessionStatus::Changed, state) => inner.update(res, state, value).await, @@ -453,14 +450,14 @@ mod test { #[actix_rt::test] async fn test_session_workflow() { // Step 1: GET index - // - set-cookie actix-session will be in response (session cookie #1) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} - // - cookie should have default max-age of 7 days - // Step 2: GET index, including session cookie #1 in request - // - set-cookie will *not* be in response - // - response should be: {"counter": 0, "user_id": None} - // Step 3: POST to do_something, including session cookie #1 in request + // Step 2: POST to do_something // - adds new session state in redis: {"counter": 1} + // - set-cookie actix-session should be in response (session cookie #1) + // - response should be: {"counter": 1, "user_id": None} + // Step 3: GET index, including session cookie #1 in request + // - set-cookie will *not* be in response // - response should be: {"counter": 1, "user_id": None} // Step 4: POST again to do_something, including session cookie #1 in request // - updates session state in redis: {"counter": 2} @@ -472,15 +469,15 @@ mod test { // - response should be: {"counter": 2, "user_id": "ferris"} // Step 7: POST again to do_something, including session cookie #2 in request // - updates session state in redis: {"counter": 3, "user_id": "ferris"} - // - response should be: {"counter": 2, "user_id": None} + // - response should be: {"counter": 3, "user_id": "ferris"} // Step 8: GET index, including session cookie #1 in request - // - set-cookie actix-session will be in response (session cookie #3) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} // Step 9: POST to logout, including session cookie #2 // - set-cookie actix-session will be in response with session cookie #2 // invalidation logic // Step 10: GET index, including session cookie #2 in request - // - set-cookie actix-session will be in response (session cookie #3) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} let srv = actix_test::start(|| { @@ -494,17 +491,11 @@ mod test { }); // Step 1: GET index - // - set-cookie actix-session will be in response (session cookie #1) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} let req_1a = srv.get("/").send(); let mut resp_1 = req_1a.await.unwrap(); - let cookie_1 = resp_1 - .cookies() - .unwrap() - .clone() - .into_iter() - .find(|c| c.name() == "test-session") - .unwrap(); + assert!(resp_1.cookies().unwrap().is_empty()); let result_1 = resp_1.json::().await.unwrap(); assert_eq!( result_1, @@ -513,26 +504,28 @@ mod test { counter: 0 } ); - assert_eq!(cookie_1.max_age(), Some(Duration::days(7))); - // Step 2: GET index, including session cookie #1 in request - // - set-cookie will *not* be in response - // - response should be: {"counter": 0, "user_id": None} - let req_2 = srv.get("/").cookie(cookie_1.clone()).send(); + // Step 2: POST to do_something + // - adds new session state in redis: {"counter": 1} + // - set-cookie actix-session should be in response (session cookie #1) + // - response should be: {"counter": 1, "user_id": None} + let req_2 = srv.post("/do_something").send(); let resp_2 = req_2.await.unwrap(); - let cookie_2 = resp_2 + let cookie_1 = resp_2 .cookies() .unwrap() .clone() .into_iter() - .find(|c| c.name() == "test-session"); - assert_eq!(cookie_2, None); + .find(|c| c.name() == "test-session") + .unwrap(); + assert_eq!(cookie_1.max_age(), Some(Duration::days(7))); - // Step 3: POST to do_something, including session cookie #1 in request - // - adds new session state in redis: {"counter": 1} + // Step 3: GET index, including session cookie #1 in request + // - set-cookie will *not* be in response // - response should be: {"counter": 1, "user_id": None} - let req_3 = srv.post("/do_something").cookie(cookie_1.clone()).send(); + let req_3 = srv.get("/").cookie(cookie_1.clone()).send(); let mut resp_3 = req_3.await.unwrap(); + assert!(resp_3.cookies().unwrap().is_empty()); let result_3 = resp_3.json::().await.unwrap(); assert_eq!( result_3, @@ -597,7 +590,7 @@ mod test { // Step 7: POST again to do_something, including session cookie #2 in request // - updates session state in redis: {"counter": 3, "user_id": "ferris"} - // - response should be: {"counter": 2, "user_id": None} + // - response should be: {"counter": 3, "user_id": "ferris"} let req_7 = srv.post("/do_something").cookie(cookie_2.clone()).send(); let mut resp_7 = req_7.await.unwrap(); let result_7 = resp_7.json::().await.unwrap(); @@ -610,17 +603,11 @@ mod test { ); // Step 8: GET index, including session cookie #1 in request - // - set-cookie actix-session will be in response (session cookie #3) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} let req_8 = srv.get("/").cookie(cookie_1.clone()).send(); let mut resp_8 = req_8.await.unwrap(); - let cookie_3 = resp_8 - .cookies() - .unwrap() - .clone() - .into_iter() - .find(|c| c.name() == "test-session") - .unwrap(); + assert!(resp_8.cookies().unwrap().is_empty()); let result_8 = resp_8.json::().await.unwrap(); assert_eq!( result_8, @@ -629,14 +616,13 @@ mod test { counter: 0 } ); - assert_ne!(cookie_3.value(), cookie_2.value()); // Step 9: POST to logout, including session cookie #2 // - set-cookie actix-session will be in response with session cookie #2 // invalidation logic let req_9 = srv.post("/logout").cookie(cookie_2.clone()).send(); let resp_9 = req_9.await.unwrap(); - let cookie_4 = resp_9 + let cookie_3 = resp_9 .cookies() .unwrap() .clone() @@ -645,17 +631,18 @@ mod test { .unwrap(); assert_ne!( OffsetDateTime::now_utc().year(), - cookie_4 + cookie_3 .expires() .map(|t| t.datetime().expect("Expiration is a datetime").year()) .unwrap() ); // Step 10: GET index, including session cookie #2 in request - // - set-cookie actix-session will be in response (session cookie #3) + // - set-cookie actix-session should NOT be in response (session data is empty) // - response should be: {"counter": 0, "user_id": None} let req_10 = srv.get("/").cookie(cookie_2.clone()).send(); let mut resp_10 = req_10.await.unwrap(); + assert!(resp_10.cookies().unwrap().is_empty()); let result_10 = resp_10.json::().await.unwrap(); assert_eq!( result_10, @@ -664,15 +651,6 @@ mod test { counter: 0 } ); - - let cookie_5 = resp_10 - .cookies() - .unwrap() - .clone() - .into_iter() - .find(|c| c.name() == "test-session") - .unwrap(); - assert_ne!(cookie_5.value(), cookie_2.value()); } #[actix_rt::test] @@ -688,10 +666,10 @@ mod test { .cookie_max_age(None), ) .wrap(middleware::Logger::default()) - .service(resource("/").route(get().to(index))) + .service(resource("/do_something").route(post().to(do_something))) }); - let req = srv.get("/").send(); + let req = srv.post("/do_something").send(); let resp = req.await.unwrap(); let cookie = resp .cookies()