http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 83 forks source link

provide a mechanism to retrieve request.ext on drop #201

Closed jbr closed 4 years ago

jbr commented 4 years ago

This patch allows http-types users to optionally retrieve ext on a channel when the request is dropped, providing a way to wire arbitrary data through a request-response cycle in situations where the request is consumed when building a response.


Context

Who would use this?

Tide currently has the following signature for endpoints, but async: Fn(Request) -> Response. A result of this is the only place that can see both the request and the response is user-written code. The framework and middleware are unable to retrieve any values set on the request's extension typemap because that typemap is dropped halfway through the request-response middleware execution path.

Why would tide middleware want this?

One example might be a struct that represents a Transaction that is not Clone. A transaction middleware might want to be able to place that transaction into the request's ext, allow other parts of the application to execute database changes against that transaction, and then if the entire response is a success when it returns to the transaction middleware, the middleware commits the transaction, and otherwise rolls it back. Currently, tide could support half of this, but the transaction would get dropped when the containing Request goes out of scope in the endpoint.

Another example is sessions — the current implementation of sessions needs to use interior mutability on a session in order to place a clone of the session into Request Extensions. If there was a way to retrieve the passed-in-session, it would avoid wrapping the data with Arc<RwLock<_>>. This would also make it easier to have a generic data without exposing lock guards as part of the api.

What about other users of http-types other than tide?

This behavior is entirely opt-in. If there is no ext_sender set on a request, the only cost is a self.project() and a conditional check on drop


Alternatives


Usage

Wiring this up in tide looks like:

diff --git a/src/middleware.rs b/src/middleware.rs
index 7e1ca9a..c3fa657 100644
--- a/src/middleware.rs
+++ b/src/middleware.rs
@@ -46,18 +46,29 @@ pub struct Next<'a, State> {

 impl<State: Clone + Send + Sync + 'static> Next<'_, State> {
     /// Asynchronously execute the remaining middleware chain.
-    pub async fn run(mut self, req: Request<State>) -> Response {
+    pub async fn run(mut self, mut req: Request<State>) -> Response {
         if let Some((current, next)) = self.next_middleware.split_first() {
             self.next_middleware = next;
             match current.handle(req, self).await {
-                Ok(request) => request,
+                Ok(response) => response,
                 Err(err) => err.into(),
             }
         } else {
-            match self.endpoint.call(req).await {
-                Ok(request) => request,
+            let (tx, rx) = async_std::sync::channel(1);
+            let inner_req: &mut http_types::Request = req.as_mut();
+            inner_req.set_ext_sender(tx);
+
+            let mut response = match self.endpoint.call(req).await {
+                Ok(response) => response,
                 Err(err) => err.into(),
+            };
+
+            if let Ok(ext) = rx.recv().await {
+                let inner_res: &mut http_types::Response = response.as_mut();
+                *inner_res.ext_mut() = ext;
             }
+
+            response
         }
     }
 }
Fishrock123 commented 4 years ago

This is very interesting, it worries me a bit, mostly in what users may expect, and so good documentation would be crucial in Tide & Surf. (Speaking of which, Surf would probably need a Response version of this.)

This certainly feels like it should violate some kind of coherency rules but upon inspection it seems reasonable, if kinda magic.

I worry a tad about perf, but I don't know that much and so long as a single channel can be kept in e.g. Next I think the overhead should be fairly low.

yoshuawuyts commented 4 years ago

Thanks for the detailed explainer @jbr! Something I'm curious about is what a concrete example of this mechanism would look like — I know you've been working on sessions, and my guess is that you have encountered a use case there? I'd be curious to learn more about how this would be used in practice.

jbr commented 4 years ago

@yoshuawuyts I'll going push a variant of both tide and async-session that uses generic inner session data instead of a HashMap<String,String> and uses this to pass that data from middleware→request→response→middleware to use as a concrete demonstration. This would simplify sessions substantially for the reasons described above

yoshuawuyts commented 4 years ago

We talked about this today in person; this is not a change we currently want make. Closing!