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

feat: allow Middleware to be added at any time #747

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Nov 20, 2020

Arc has thing kind-of-magic method called make_mut which can do an interior clone if there are immutable references.

This uses that to allow middleware to be registered on a server while it is already listening, without interfering with existing requests.


To be fair, I'm not 100% sure we should actually do this since supporting this use-case seems a bit questionable but if anyone finds it useful we can totally do it...

Edit: note that this does not cover doing the same for routes, which would require Router to be Clone.

`Arc` has thing kind-of-magic method called `make_mut` which can do an interior clone if there are immutable references.

This uses that to allow middleware to be registered on a server while it is already listening, without interfering with existing requests.
@yoshuawuyts
Copy link
Member

@Fishrock123 I don't believe this will work: this will create "unique ownership" which means that when new middleware is pushed, it's pushed onto a different middleware stack than the one that's used, and it effectively does nothing. In order to enable middleware to be pushed after the server has started, we should use an Arc<Mutex<Middleware>> or an Arc<RwLock<Middleware>>.

But those changes have significant performance implications. So I'd like to get a good sense what's motivating this change first.

@Fishrock123
Copy link
Member Author

Fishrock123 commented Nov 27, 2020

I would have thought that too, but that's not what the std lib examples shows:

use std::sync::Arc;

let mut data = Arc::new(5);

*Arc::make_mut(&mut data) += 1;         // Won't clone anything
let mut other_data = Arc::clone(&data); // Won't clone inner data
*Arc::make_mut(&mut data) += 1;         // Clones inner data
*Arc::make_mut(&mut data) += 1;         // Won't clone anything
*Arc::make_mut(&mut other_data) *= 2;   // Won't clone anything

// Now `data` and `other_data` point to different allocations.
assert_eq!(*data, 8);
assert_eq!(*other_data, 12);

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Dec 4, 2020

@Fishrock123 that example shows that data and other_data hold different values. For this to work for us they would need to hold the same values. Because we clone the middleware on each incoming request, we would continuously hit this case:

*Arc::make_mut(&mut data) += 1;         // Clones inner data

which is what brings the middleware out of sync. Pushing onto this would require a clone, and then push -- which creates a separate middleware stack.

For shared ownership to work this would need to be an Arc<Mutex<Middleware>> which can provide unique access to the middleware, update it, and then enable others to read from it. If it had the semantics you describe it would consitute a data race, which is something Rust is designed to disallow.

@Fishrock123
Copy link
Member Author

I think that would be a feature though? Inserting a middleware when a request is potentially on the way out is a good way to cause indecipherable timing errors.

This would copy the stack if necessary if a middleware was added late, and then all future requests would use that without interfering with existing requests.

That said I'm still not really convinced it's a fundamentally good idea, but this shows it can be done like this.

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

Successfully merging this pull request may close these issues.

2 participants