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

Shouldn't Middleware functions take &mut self? #48

Closed
bIgBV opened this issue Nov 18, 2018 · 7 comments
Closed

Shouldn't Middleware functions take &mut self? #48

bIgBV opened this issue Nov 18, 2018 · 7 comments

Comments

@bIgBV
Copy link
Contributor

bIgBV commented Nov 18, 2018

Is there any reason why middleware functions cannot take &mut self? Middleware could enable storing some state when a request comes in and then use that state during the response phase. I'm facing the same issue right now when implementing #8. I figured that at the minimum when someone firsts starts using Tide, they should see a simple log in the terminal of the format:

<timestamp> <http method> <path> <statuscode>

But in order to do so, I need to store method and path information which is only present on the Request object and access them when the response function is called by the call method in Server.

I imagine that there are many such usecases where middleware would be storing information when the request method is called. An example that comes to mind is a caching middleware.

Therefore, I think that the request and response methods of the Middleware trait should take &mut self. Or is there a better way to go about solving this?

@tzilist tzilist closed this as completed Nov 19, 2018
@tzilist tzilist reopened this Nov 19, 2018
@tzilist
Copy link
Contributor

tzilist commented Nov 19, 2018

Sorry accidentally closed this! My bad!

Meant to leave this comment:

It might be worth storing a optional mutable context that is passed along with each request as opposed to just the middleware itself? That'll allow devs to separate out the functionality of the middleware vs what needs to be stored on each request? I dunno - just a thought!

@bIgBV
Copy link
Contributor Author

bIgBV commented Nov 19, 2018

@tzilist that's a good idea. I initially considered it, but I'm not entirely sure what the current discussion around adding context is, but it would definitely be extremely useful. As you could use the context object to pass information down to the endpoints as well.

@tirr-c
Copy link
Collaborator

tirr-c commented Nov 19, 2018

Middleware may be shared among multiple threads, so we can't take a mutable reference (it leads to data races). Instead, you can use some sync types for middleware state.

@bIgBV
Copy link
Contributor Author

bIgBV commented Nov 19, 2018

@tirr-c I did not realize that. Correct me if I'm wrong, but I'm guessing the reason Middleware may be shared amongst threads is because the executors themselves are running in a threadpool and the futures could be spawned on any of those threads right?

If that's the case, then I have to put the shared state behind a Mutex, right?

@aturon
Copy link
Collaborator

aturon commented Nov 19, 2018

@bIgBV correct.

I think it'd actually be a good idea to open an issue for talking about the threading model in general -- I haven't put a ton of thought into it and there are lot of potential options.

@bIgBV
Copy link
Contributor Author

bIgBV commented Nov 19, 2018

@aturon what do you mean by the threading model? Isn't Tide using hyper internally, meaning it's running on tokio? How can we have a different threading model than the one on top of which we are building? Or am I not understanding this right?

@bIgBV
Copy link
Contributor Author

bIgBV commented Nov 19, 2018

Just opened #50 and my original question has been answered, so closing this. issue.

@bIgBV bIgBV closed this as completed Nov 19, 2018
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

4 participants