Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider generalising middleware by passing state into every call #816

Closed
jsdw opened this issue Jul 6, 2022 · 3 comments
Closed

Consider generalising middleware by passing state into every call #816

jsdw opened this issue Jul 6, 2022 · 3 comments

Comments

@jsdw
Copy link
Collaborator

jsdw commented Jul 6, 2022

Currently the middleware is quite restrictive in what it enables with its passing of a copyable Instant from one method to another.

Is it worth generalising the way that we pass state between middleware calls to give people more flexibility?

Some thoughts on things one might want to do:

  • Log the number of requests per connection (logs on disconnect)
  • Log the average time taken per request on a connection
  • Log the time that the connection was active for.

It might be that we don't need to extend middleware to support this kind of thing though?

See #793 (comment)

@jsdw jsdw changed the title Generalise middleware + state Consider generalising middleware by passing state into every call Jul 6, 2022
@jsdw jsdw added this to the v1.0 milestone Jul 6, 2022
@jsdw
Copy link
Collaborator Author

jsdw commented Jul 6, 2022

I added this to the 1.0 milestone because it's a breaking API change, but we may decide we don't want to do anything, and that's ok too!

@niklasad1
Copy link
Member

niklasad1 commented Jul 8, 2022

So in general I like the idea but it's quite involved to implement as it requires major refactoring.

AFAIU it will work like this for each connection

// one state per connection
let state = <M as Middleware>::State::default();

middleware.on_connect(remote_addr, request.headers(), &mut state);

while active_connection {
   let single_or_batch = rx();

   // calls on `Middlware::on_call` and `Middleware::on_response`
   // this won't work because we can't borrow state mutable more than once
   // so we would need a mutex for that
   spawn(execute_call_async(middleware, &mut state, single_or_batch));
}

middleware.on_disconnect(remote_addr, &mut state);

The neat thing about the current design is that each "method call/request" gets it's own state and it's not mutated so we don't
need to synchronize it.

Also State in the design can't be clone anymore as with different state if it's cloned before or after it's mutated as each method requires mut State in the "new design suggestion".

Thus, I'm a bit skeptical about this approach as Mutex would be quite expensive and impl<A, B> HttpMiddleware for (A, B) won't work as well if State: !Clone

Can't we just provide a ConnectionInstant and RequestInstant instead or other ideas?

@jsdw
Copy link
Collaborator Author

jsdw commented Aug 2, 2022

Going to close this; I don't think it's worth the time to dig into it if the current approach does what we need :)

@jsdw jsdw closed this as completed Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants