From fa2a9401881453701c107c6628697a06ea35ed96 Mon Sep 17 00:00:00 2001 From: Jonathan Irhodia Date: Thu, 26 Sep 2024 10:21:22 +0100 Subject: [PATCH 1/4] chore: add info to explain how arc to box works (#195) * chore: add info to explain how arc to box works * add comments * update state comments --- crates/shared/src/server/context.rs | 13 +++- crates/shared/src/traits/controller_trait.rs | 66 +++++++++++++++++--- 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/crates/shared/src/server/context.rs b/crates/shared/src/server/context.rs index 02ab9cc..4097027 100644 --- a/crates/shared/src/server/context.rs +++ b/crates/shared/src/server/context.rs @@ -36,14 +36,19 @@ impl AppState for Box { } } +/// # Panics +/// Panics if the state has been dropped. This should never happen unless the state is dropped manually. impl From<&Arc>> for Box { fn from(value: &Arc>) -> Self { + // creating a clone is essential since this ref will be dropped after this function returns let arc_clone = value.clone(); let state_ref: &dyn AppState = &**arc_clone; let state_ptr: *const dyn AppState = state_ref as *const dyn AppState; - let nn_ptr = std::ptr::NonNull::new(state_ptr as *mut dyn AppState).unwrap(); + // SAFETY: state_ptr is not null, it is safe to convert it to a NonNull pointer, this way we can safely convert it back to a Box + let nn_ptr = std::ptr::NonNull::new(state_ptr as *mut dyn AppState) + .expect("State has been dropped, ensure it is being cloned correctly."); // This should never happen, if it does, it's a bug let raw_ptr = nn_ptr.as_ptr(); unsafe { Box::from_raw(raw_ptr) } @@ -448,12 +453,14 @@ impl NgynContext { /// context.execute(&mut response).await; /// ``` pub(crate) async fn execute(&mut self, res: &mut NgynResponse) { + // safely consume the route information, it will be set again if needed let (handler, controller) = match self.route_info.take() { Some((handler, ctrl)) => (handler, ctrl), None => return, }; - let mut controller = - ManuallyDrop::>::new(controller.clone().into()); + // allow the controller to live even after the request is handled, until the server is stopped or crashes or in weird cases, the controller is dropped. + // If the controller is dropped, the server will panic. + let mut controller = ManuallyDrop::>::new(controller.into()); // panics if the controller has been dropped controller.handle(&handler, self, res).await; } } diff --git a/crates/shared/src/traits/controller_trait.rs b/crates/shared/src/traits/controller_trait.rs index a04b82e..e38fbdd 100644 --- a/crates/shared/src/traits/controller_trait.rs +++ b/crates/shared/src/traits/controller_trait.rs @@ -31,18 +31,68 @@ pub trait NgynController: NgynInjectable + Sync + Send { } /// In Ngyn, controllers are stored as `Arc>`. -/// This is because controllers are shared across threads and need to be cloned easily. +/// And we do this because controllers are shared across threads and an arc guarantees +/// that the controller is not dropped until all references to it are dropped. /// -/// Here's how we convert an `Arc>` to a `Box`. -/// This conversion allows us to mutably borrow the controller and handle routing logic. +/// When working with controllers, you'd quickly notice that Ngyn allows you to define routes that require mutable access to the controller. +/// For instance, take this sample controller: +/// ```rust ignore +/// #[controller] +/// struct TestController; +/// +/// #[routes] +/// impl TestController { +/// #[get("/")] +/// async fn index(&mut self) -> String { +/// "Hello, World!".to_string() +/// } +/// } +/// ``` +/// +/// In the above example, the `index` method requires mutable access to the controller. This pattern, though not encouraged (check app states), is allowed in Ngyn. +/// You could for instance create a localized state in the controller that is only accessible to the controller and its routes. +/// The way Ngyn allows this without performance overhead is through a specialized `Arc -> Box` conversion that only works so well becasue of how Ngyn is designed. +/// +/// HOW DOES IT WORK? +/// +/// ```text +/// +-----------------+ +-----------------+ +-----------------+ +/// | Arc> | | Arc> | | Arc> | +/// +-----------------+ +-----------------+ +-----------------+ +/// | | | +/// +-----------------+ +-----------------+ +-----------------+ +/// | &Box | | &Box | | &Box | +/// +-----------------+ +-----------------+ +-----------------+ +/// | | | +/// +-----------------+ +-----------------+ +-----------------+ +/// | &mut Ctrl | | &mut Ctrl | | &mut Ctrl | +/// +-----------------+ +-----------------+ +-----------------+ +/// | | | +/// +-----------------+ +-----------------+ +-----------------+ +/// | *mut Ctrl | | *mut Ctrl | | *mut Ctrl | +/// +-----------------+ +-----------------+ +-----------------+ +/// | | | +/// +-----------------+ +-----------------+ +-----------------+ +/// | Box | | Box | | Box | +/// +-----------------+ +-----------------+ +-----------------+ +/// +/// ``` +/// +/// +/// When a controller is created, we box it and then wrap it in an Arc. This way, the controller is converted to a trait object and can be shared across threads. +/// The trait object is what allows us to call the controller's methods from the server. But when we need mutable access to the controller, we convert it back to a Box. +/// Rather than making use of a mutex, what we do is get the raw pointer of the initial controller, ensure it's not null, and then convert it back to a Box. +/// +/// # Panics +/// Panics if the controller has been dropped. This should never happen unless the controller is dropped manually. impl From>> for Box { - fn from(arc: Arc>) -> Self { - let arc_clone = arc.clone(); - let controller_ref: &dyn NgynController = &**arc_clone; - + fn from(controller_arc: Arc>) -> Self { + let controller_ref: &dyn NgynController = &**controller_arc; let controller_ptr: *const dyn NgynController = controller_ref as *const dyn NgynController; - let nn_ptr = NonNull::new(controller_ptr as *mut dyn NgynController).unwrap(); + // SAFETY: controller_ptr is not null, it is safe to convert it to a NonNull pointer, this way we can safely convert it back to a Box + let nn_ptr = NonNull::new(controller_ptr as *mut dyn NgynController) + .expect("Controller has been dropped, ensure it is being cloned correctly."); let raw_ptr = nn_ptr.as_ptr(); unsafe { Box::from_raw(raw_ptr) } From 293b529be9a71242e325b6ecfbe006e73010f3d9 Mon Sep 17 00:00:00 2001 From: Jonathan Irhodia Date: Sat, 28 Sep 2024 18:49:22 +0100 Subject: [PATCH 2/4] feat: support for async middleware (#197) * feat: support for async middleware * fix middleware test code * fmt --- crates/macros/src/common/controller.rs | 2 +- crates/shared/src/core/engine.rs | 8 ++--- crates/shared/src/core/handler.rs | 23 +------------- crates/shared/src/server/mod.rs | 2 +- crates/shared/src/traits/controller_trait.rs | 2 +- crates/shared/src/traits/middleware_trait.rs | 31 +++++++++++++++++-- .../src/middlewares/notfound_middleware.rs | 2 +- .../src/middlewares/test_middleware.rs | 2 +- 8 files changed, 39 insertions(+), 33 deletions(-) diff --git a/crates/macros/src/common/controller.rs b/crates/macros/src/common/controller.rs index fd02d9c..016a5a8 100644 --- a/crates/macros/src/common/controller.rs +++ b/crates/macros/src/common/controller.rs @@ -131,7 +131,7 @@ pub(crate) fn controller_macro(args: TokenStream, input: TokenStream) -> TokenSt quote! { let mut middleware = #m::default(); middleware.inject(cx); - middleware.handle(cx, res); + middleware.handle(cx, res).await; } }) .collect(); diff --git a/crates/shared/src/core/engine.rs b/crates/shared/src/core/engine.rs index ba88a70..313efc5 100644 --- a/crates/shared/src/core/engine.rs +++ b/crates/shared/src/core/engine.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use super::RouteHandler; use crate::{ server::{context::AppState, Method, Middlewares, NgynContext, NgynResponse, Routes}, - traits::{NgynController, NgynInterpreter, NgynMiddleware, NgynModule}, + traits::{Middleware, NgynController, NgynInterpreter, NgynMiddleware, NgynModule}, }; #[derive(Default)] @@ -48,7 +48,7 @@ impl PlatformData { // trigger global middlewares for middleware in &self.middlewares { - middleware.handle(&mut cx, &mut res); + middleware.run(&mut cx, &mut res).await; } // execute controlled route if it is handled @@ -96,7 +96,7 @@ impl PlatformData { /// ### Arguments /// /// * `middleware` - The middleware to add. - pub(self) fn add_middleware(&mut self, middleware: Box) { + pub(self) fn add_middleware(&mut self, middleware: Box) { self.middlewares.push(middleware); } @@ -272,7 +272,7 @@ mod tests { } impl NgynMiddleware for MockMiddleware { - fn handle(&self, _cx: &mut NgynContext, _res: &mut NgynResponse) {} + async fn handle(&self, _cx: &mut NgynContext, _res: &mut NgynResponse) {} } struct MockInterpreter; diff --git a/crates/shared/src/core/handler.rs b/crates/shared/src/core/handler.rs index 79e8131..d55b050 100644 --- a/crates/shared/src/core/handler.rs +++ b/crates/shared/src/core/handler.rs @@ -1,9 +1,6 @@ use std::{future::Future, pin::Pin}; -use crate::{ - server::{NgynContext, NgynResponse, ToBytes}, - traits::NgynMiddleware, -}; +use crate::server::{NgynContext, NgynResponse, ToBytes}; /// Represents a handler function that takes in a mutable reference to `NgynContext` and `NgynResponse`. pub type Handler = dyn Fn(&mut NgynContext, &mut NgynResponse) + Send + Sync + 'static; @@ -76,21 +73,3 @@ pub fn async_handler + Send + 'sta }) }) } - -pub fn middleware(middleware: M) -> Box { - Box::new(move |ctx: &mut NgynContext, res: &mut NgynResponse| { - middleware.handle(ctx, res); - }) -} - -pub fn with_middlewares( - middleware: Vec, - handler: Box, -) -> impl Into { - Box::new(move |ctx: &mut NgynContext, res: &mut NgynResponse| { - for m in &middleware { - m.handle(ctx, res); - } - handler(ctx, res); - }) -} diff --git a/crates/shared/src/server/mod.rs b/crates/shared/src/server/mod.rs index 9dfc0f4..c1a740d 100644 --- a/crates/shared/src/server/mod.rs +++ b/crates/shared/src/server/mod.rs @@ -16,4 +16,4 @@ pub type NgynRequest = http::Request>; pub type NgynResponse = http::Response>; pub(crate) type Routes = Vec<(String, Option, Box)>; -pub(crate) type Middlewares = Vec>; +pub(crate) type Middlewares = Vec>; diff --git a/crates/shared/src/traits/controller_trait.rs b/crates/shared/src/traits/controller_trait.rs index e38fbdd..9bfeb63 100644 --- a/crates/shared/src/traits/controller_trait.rs +++ b/crates/shared/src/traits/controller_trait.rs @@ -75,7 +75,7 @@ pub trait NgynController: NgynInjectable + Sync + Send { /// +-----------------+ +-----------------+ +-----------------+ /// | Box | | Box | | Box | /// +-----------------+ +-----------------+ +-----------------+ -/// +/// /// ``` /// /// diff --git a/crates/shared/src/traits/middleware_trait.rs b/crates/shared/src/traits/middleware_trait.rs index 4e1fb3f..bc5ea54 100644 --- a/crates/shared/src/traits/middleware_trait.rs +++ b/crates/shared/src/traits/middleware_trait.rs @@ -1,3 +1,5 @@ +use std::{future::Future, pin::Pin}; + use crate::{ server::{NgynContext, NgynResponse}, traits::NgynInjectable, @@ -29,12 +31,37 @@ use crate::{ /// } /// /// impl NgynMiddleware for RequestReceivedLogger { -/// fn handle(&self, cx: &mut NgynContext, res: &mut NgynResponse) { +/// async fn handle(&self, cx: &mut NgynContext, res: &mut NgynResponse) { /// println!("Request received: {:?}", cx.request()); /// } /// } /// ``` pub trait NgynMiddleware: NgynInjectable + Sync { /// Handles the request. - fn handle(&self, cx: &mut NgynContext, res: &mut NgynResponse); + #[allow(async_fn_in_trait)] + fn handle( + &self, + cx: &mut NgynContext, + res: &mut NgynResponse, + ) -> impl std::future::Future + Send; +} + +pub(crate) trait Middleware: NgynInjectable + Sync { + fn run<'a>( + &'a self, + _cx: &'a mut NgynContext, + _res: &'a mut NgynResponse, + ) -> Pin + Send + 'a>> { + Box::pin(async move {}) + } +} + +impl Middleware for T { + fn run<'a>( + &'a self, + cx: &'a mut NgynContext, + res: &'a mut NgynResponse, + ) -> Pin + Send + 'a>> { + Box::pin(self.handle(cx, res)) + } } diff --git a/examples/weather_app/src/middlewares/notfound_middleware.rs b/examples/weather_app/src/middlewares/notfound_middleware.rs index 03ef306..f16ed6f 100644 --- a/examples/weather_app/src/middlewares/notfound_middleware.rs +++ b/examples/weather_app/src/middlewares/notfound_middleware.rs @@ -5,7 +5,7 @@ use serde_json::json; pub struct NotFoundMiddleware; impl NgynMiddleware for NotFoundMiddleware { - fn handle(&self, cx: &mut NgynContext, res: &mut NgynResponse) { + async fn handle(&self, cx: &mut NgynContext, res: &mut NgynResponse) { if cx.is_valid_route() { return; } diff --git a/examples/weather_app/src/middlewares/test_middleware.rs b/examples/weather_app/src/middlewares/test_middleware.rs index 9f50128..45111e9 100644 --- a/examples/weather_app/src/middlewares/test_middleware.rs +++ b/examples/weather_app/src/middlewares/test_middleware.rs @@ -4,7 +4,7 @@ use ngyn::prelude::*; pub struct TestMiddleware; impl NgynMiddleware for TestMiddleware { - fn handle(&self, _cx: &mut NgynContext, _response: &mut NgynResponse) { + async fn handle(&self, _cx: &mut NgynContext, _response: &mut NgynResponse) { println!("middleware works"); } } From 82c412f4c9186c5f970f0f11aedea5d59eb3cc53 Mon Sep 17 00:00:00 2001 From: Jonathan Irhodia Date: Sat, 28 Sep 2024 22:25:20 +0100 Subject: [PATCH 3/4] feat: add redirect handlers (#202) --- crates/shared/src/core/engine.rs | 6 ++-- crates/shared/src/core/handler.rs | 51 +++++++++++++++++++++++++++++-- crates/shared/src/server/mod.rs | 3 -- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/crates/shared/src/core/engine.rs b/crates/shared/src/core/engine.rs index 313efc5..22f52cf 100644 --- a/crates/shared/src/core/engine.rs +++ b/crates/shared/src/core/engine.rs @@ -5,14 +5,14 @@ use std::sync::Arc; use super::RouteHandler; use crate::{ - server::{context::AppState, Method, Middlewares, NgynContext, NgynResponse, Routes}, + server::{context::AppState, Method, NgynContext, NgynResponse}, traits::{Middleware, NgynController, NgynInterpreter, NgynMiddleware, NgynModule}, }; #[derive(Default)] pub struct PlatformData { - routes: Routes, - middlewares: Middlewares, + routes: Vec<(String, Option, Box)>, + middlewares: Vec>, interpreters: Vec>, state: Option>>, } diff --git a/crates/shared/src/core/handler.rs b/crates/shared/src/core/handler.rs index d55b050..82540a7 100644 --- a/crates/shared/src/core/handler.rs +++ b/crates/shared/src/core/handler.rs @@ -1,11 +1,13 @@ use std::{future::Future, pin::Pin}; +use http::{HeaderValue, StatusCode}; + use crate::server::{NgynContext, NgynResponse, ToBytes}; /// Represents a handler function that takes in a mutable reference to `NgynContext` and `NgynResponse`. -pub type Handler = dyn Fn(&mut NgynContext, &mut NgynResponse) + Send + Sync + 'static; +pub(crate) type Handler = dyn Fn(&mut NgynContext, &mut NgynResponse) + Send + Sync + 'static; -pub type AsyncHandler = Box< +pub(crate) type AsyncHandler = Box< dyn for<'a, 'b> Fn( &'a mut NgynContext, &'b mut NgynResponse, @@ -73,3 +75,48 @@ pub fn async_handler + Send + 'sta }) }) } + +/// Create a not-implemented handler that returns a `501 Not Implemented` status code. +/// +/// This is very similar to unimplemented! macro in Rust. +pub fn not_implemented() -> Box { + Box::new(|_ctx: &mut NgynContext, res: &mut NgynResponse| { + *res.status_mut() = StatusCode::NOT_IMPLEMENTED; + }) +} + +/// Redirects to a specified location with a `303 See Other` status code. +pub fn redirect_to(location: &'static str) -> Box { + Box::new(|_ctx: &mut NgynContext, res: &mut NgynResponse| { + res.headers_mut() + .insert("Location", HeaderValue::from_str(location).unwrap()); + *res.status_mut() = StatusCode::SEE_OTHER; + }) +} + +/// Redirects to a specified location with a `307 Temporary Redirect` status code. +pub fn redirect_temporary(location: &'static str) -> Box { + Box::new(|_ctx: &mut NgynContext, res: &mut NgynResponse| { + res.headers_mut() + .insert("Location", HeaderValue::from_str(location).unwrap()); + *res.status_mut() = StatusCode::TEMPORARY_REDIRECT; + }) +} + +/// Redirects to a specified location with a `301 Moved Permanently` status code. +pub fn redirect_permanent(location: &'static str) -> Box { + Box::new(|_ctx: &mut NgynContext, res: &mut NgynResponse| { + res.headers_mut() + .insert("Location", HeaderValue::from_str(location).unwrap()); + *res.status_mut() = StatusCode::MOVED_PERMANENTLY; + }) +} + +/// Redirects to a specified location with a `302 Found` status code. +pub fn redirect_found(location: &'static str) -> Box { + Box::new(|_ctx: &mut NgynContext, res: &mut NgynResponse| { + res.headers_mut() + .insert("Location", HeaderValue::from_str(location).unwrap()); + *res.status_mut() = StatusCode::FOUND; + }) +} diff --git a/crates/shared/src/server/mod.rs b/crates/shared/src/server/mod.rs index c1a740d..33309a0 100644 --- a/crates/shared/src/server/mod.rs +++ b/crates/shared/src/server/mod.rs @@ -14,6 +14,3 @@ pub use transformer::{Body, Param, Query, Transducer, Transformer}; pub type NgynRequest = http::Request>; pub type NgynResponse = http::Response>; - -pub(crate) type Routes = Vec<(String, Option, Box)>; -pub(crate) type Middlewares = Vec>; From 6e40dc3c6ce010575a76ee14191277f35e607ece Mon Sep 17 00:00:00 2001 From: Jonathan Irhodia Date: Sun, 29 Sep 2024 17:02:48 +0100 Subject: [PATCH 4/4] chore: add reexports for handlers (#203) --- crates/core/src/lib.rs | 2 +- crates/shared/src/core/engine.rs | 6 +++--- crates/shared/src/core/handler.rs | 2 +- crates/shared/src/core/mod.rs | 7 ++----- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index e7f19bf..5c510e3 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -17,7 +17,7 @@ pub mod prelude { pub use crate::macros::*; pub use ngyn_hyper::HyperApplication; pub use ngyn_shared::{ - core::{handler, NgynEngine}, + core::{handler::*, engine::NgynEngine}, server::{ Body, JsonResponse, JsonResult, NgynContext, NgynRequest, NgynResponse, Param, Query, Transducer, diff --git a/crates/shared/src/core/engine.rs b/crates/shared/src/core/engine.rs index 22f52cf..bdd1beb 100644 --- a/crates/shared/src/core/engine.rs +++ b/crates/shared/src/core/engine.rs @@ -3,7 +3,7 @@ use http::Request; use http_body_util::Full; use std::sync::Arc; -use super::RouteHandler; +use super::handler::RouteHandler; use crate::{ server::{context::AppState, Method, NgynContext, NgynResponse}, traits::{Middleware, NgynController, NgynInterpreter, NgynMiddleware, NgynModule}, @@ -11,7 +11,7 @@ use crate::{ #[derive(Default)] pub struct PlatformData { - routes: Vec<(String, Option, Box)>, + routes: Vec<(String, Option, Box)>, middlewares: Vec>, interpreters: Vec>, state: Option>>, @@ -243,7 +243,7 @@ pub trait NgynEngine: NgynPlatform { #[cfg(test)] mod tests { - use crate::{core::Handler, traits::NgynInjectable}; + use crate::{core::handler::Handler, traits::NgynInjectable}; use std::any::Any; use super::*; diff --git a/crates/shared/src/core/handler.rs b/crates/shared/src/core/handler.rs index 82540a7..eba34b1 100644 --- a/crates/shared/src/core/handler.rs +++ b/crates/shared/src/core/handler.rs @@ -29,7 +29,7 @@ impl From impl From for RouteHandler { fn from(f: AsyncHandler) -> Self { - RouteHandler::Async(Box::new(f)) + RouteHandler::Async(f) } } diff --git a/crates/shared/src/core/mod.rs b/crates/shared/src/core/mod.rs index 87d6a85..d36d448 100644 --- a/crates/shared/src/core/mod.rs +++ b/crates/shared/src/core/mod.rs @@ -1,5 +1,2 @@ -mod engine; -mod handler; - -pub use engine::*; -pub use handler::*; +pub mod engine; +pub mod handler;