Skip to content

Commit

Permalink
refactor: remove unnecessary Arc in SessionAllocator (#38)
Browse files Browse the repository at this point in the history
### Description
Previously the `SessionAllocator` was designed to have an `Arc<dyn
StringStore>`, which caused a [Clippy issue regarding internal of Arc
not being Send or
Sync](https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync).
For the use case of `SessionAllocator`, it doesn't need to have an
internal `Arc` to the `StringStore`, a simple reference seems to be
enough.
  • Loading branch information
augustoccesar authored Oct 23, 2023
1 parent 5a8458a commit 534aa59
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 28 deletions.
7 changes: 3 additions & 4 deletions linkup-cli/src/local_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ async fn linkup_config_handler(
string_store: web::Data<MemoryStringStore>,
req_body: web::Bytes,
) -> impl Responder {
let sessions = SessionAllocator::new(string_store.into_inner());

let input_json_conf = match String::from_utf8(req_body.to_vec()) {
Ok(input_json_conf) => input_json_conf,
Err(_) => {
Expand All @@ -35,6 +33,7 @@ async fn linkup_config_handler(

match update_session_req_from_json(input_json_conf) {
Ok((desired_name, server_conf)) => {
let sessions = SessionAllocator::new(string_store.as_ref());
let session_name = sessions
.store_session(server_conf, NameKind::Animal, desired_name)
.await;
Expand All @@ -59,7 +58,7 @@ async fn linkup_ws_request_handler(
req: HttpRequest,
req_stream: web::Payload,
) -> impl Responder {
let sessions = SessionAllocator::new(string_store.into_inner());
let sessions = SessionAllocator::new(string_store.as_ref());

let url = format!("http://localhost:{}{}", LINKUP_LOCALSERVER_PORT, req.uri());
let mut headers = LinkupHeaderMap::from_actix_request(&req);
Expand Down Expand Up @@ -161,7 +160,7 @@ async fn linkup_request_handler(
req: HttpRequest,
req_body: web::Bytes,
) -> impl Responder {
let sessions = SessionAllocator::new(string_store.into_inner());
let sessions = SessionAllocator::new(string_store.as_ref());

let url = format!("http://localhost:{}{}", LINKUP_LOCALSERVER_PORT, req.uri());
let mut headers = LinkupHeaderMap::from_actix_request(&req);
Expand Down
11 changes: 6 additions & 5 deletions linkup/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,6 @@ fn extrace_tracestate(tracestate: &str, linkup_key: String) -> String {

#[cfg(test)]
mod tests {
use std::sync::Arc;

use super::*;

const CONF_STR: &str = r#"
Expand Down Expand Up @@ -325,7 +323,8 @@ mod tests {

#[tokio::test]
async fn test_get_request_session_by_subdomain() {
let sessions = SessionAllocator::new(Arc::new(MemoryStringStore::new()));
let string_store = MemoryStringStore::new();
let sessions = SessionAllocator::new(&string_store);

let config_value: serde_json::Value = serde_json::from_str(CONF_STR).unwrap();
let config: Session = config_value.try_into().unwrap();
Expand Down Expand Up @@ -455,7 +454,8 @@ mod tests {

#[tokio::test]
async fn test_get_target_url() {
let sessions = SessionAllocator::new(Arc::new(MemoryStringStore::new()));
let string_store = MemoryStringStore::new();
let sessions = SessionAllocator::new(&string_store);

let input_config_value: serde_json::Value = serde_json::from_str(CONF_STR).unwrap();
let input_config: Session = input_config_value.try_into().unwrap();
Expand Down Expand Up @@ -544,7 +544,8 @@ mod tests {

#[tokio::test]
async fn test_repeatable_rewritten_routes() {
let sessions = SessionAllocator::new(Arc::new(MemoryStringStore::new()));
let string_store = MemoryStringStore::new();
let sessions = SessionAllocator::new(&string_store);

let input_config_value: serde_json::Value = serde_json::from_str(CONF_STR).unwrap();
let input_config: Session = input_config_value.try_into().unwrap();
Expand Down
10 changes: 4 additions & 6 deletions linkup/src/session_allocator.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
use std::sync::Arc;

use crate::{
extract_tracestate_session, first_subdomain, headers::HeaderName, random_animal,
random_six_char, session_to_json, ConfigError, HeaderMap, NameKind, Session, SessionError,
StringStore,
};

pub struct SessionAllocator {
store: Arc<dyn StringStore>,
pub struct SessionAllocator<'a> {
store: &'a dyn StringStore,
}

impl SessionAllocator {
pub fn new(store: Arc<dyn StringStore>) -> Self {
impl<'a> SessionAllocator<'a> {
pub fn new(store: &'a dyn StringStore) -> Self {
Self { store }
}

Expand Down
24 changes: 12 additions & 12 deletions worker/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
// TODO(augustoccesar)[2023-10-16]: We need to revisit the session allocator and how we are using Arc.
// It seems that `CfWorkerStringStore` is not really Send or Sync, which is making so that clippy warn about this.
// For more info check: https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
#![allow(clippy::arc_with_non_send_sync)]

use http_util::*;
use kv_store::CfWorkerStringStore;
use linkup::{HeaderMap as LinkupHeaderMap, *};
use regex::Regex;
use std::sync::Arc;
use worker::*;
use ws::linkup_ws_handler;

Expand All @@ -16,7 +10,10 @@ mod kv_store;
mod utils;
mod ws;

async fn linkup_session_handler(mut req: Request, sessions: SessionAllocator) -> Result<Response> {
async fn linkup_session_handler<'a>(
mut req: Request,
sessions: &'a SessionAllocator<'a>,
) -> Result<Response> {
let body_bytes = match req.bytes().await {
Ok(bytes) => bytes,
Err(_) => return plaintext_error("Bad or missing request body", 400),
Expand Down Expand Up @@ -84,7 +81,10 @@ async fn set_cached_req(
Ok(resp)
}

async fn linkup_request_handler(mut req: Request, sessions: SessionAllocator) -> Result<Response> {
async fn linkup_request_handler<'a>(
mut req: Request,
sessions: &'a SessionAllocator<'a>,
) -> Result<Response> {
let url = match req.url() {
Ok(url) => url.to_string(),
Err(_) => return plaintext_error("Bad or missing request url", 400),
Expand Down Expand Up @@ -154,17 +154,17 @@ pub async fn main(req: Request, env: Env, _ctx: worker::Context) -> Result<Respo

let string_store = CfWorkerStringStore::new(kv);

let sessions = SessionAllocator::new(Arc::new(string_store));
let sessions = SessionAllocator::new(&string_store);

if let Ok(Some(upgrade)) = req.headers().get("upgrade") {
if upgrade == "websocket" {
return linkup_ws_handler(req, sessions).await;
return linkup_ws_handler(req, &sessions).await;
}
}

if req.method() == Method::Post && req.path() == "/linkup" {
return linkup_session_handler(req, sessions).await;
return linkup_session_handler(req, &sessions).await;
}

linkup_request_handler(req, sessions).await
linkup_request_handler(req, &sessions).await
}
5 changes: 4 additions & 1 deletion worker/src/ws.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ use futures::{

use crate::http_util::plaintext_error;

pub async fn linkup_ws_handler(req: Request, sessions: SessionAllocator) -> Result<Response> {
pub async fn linkup_ws_handler<'a>(
req: Request,
sessions: &'a SessionAllocator<'a>,
) -> Result<Response> {
let url = match req.url() {
Ok(url) => url.to_string(),
Err(_) => return plaintext_error("Bad or missing request url", 400),
Expand Down

0 comments on commit 534aa59

Please sign in to comment.