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

refactor(service): change Service::call to take &self #3223

Merged
merged 1 commit into from
May 15, 2023

Conversation

rob2244
Copy link
Contributor

@rob2244 rob2244 commented May 12, 2023

change Service::call to take &self instead of &mut self. Because of this change, the trait bound in the service::util::service_fn and the trait bound in the impl for the ServiceFn struct were changed from FnMut to Fn. This change was decided on for the following reasons:

  • It prepares the way for async fn, since then the future only borrows &self, and thus a Service can concurrently handle multiple outstanding requests at once.
  • It's clearer that Services can likely be cloned
  • To share state across clones you generally need Arc<Mutex<_>> that means you're not really using the &mut self and could do with a &self

This commit closses issue: #3040
BREAKING CHANGE: The Service::call function no longer takes a mutable reference to self. The FnMut trait bound on the service::util::service_fn function and the trait bound on the impl for the ServiceFn struct were changed from FnMut to Fn

change Service::call to take &self instead of &mut self.
Because of this change, the trait bound in the service::util::service_fn and
the trait bound in the impl for the ServiceFn struct were changed from FnMut to Fn.
This change was decided on for the following reasons:
- It prepares the way for async fn,
  since then the future only borrows &self, and thus a Service can concurrently handle
  multiple outstanding requests at once.
- It's clearer that Services can likely be cloned
- To share state across clones you generally need Arc<Mutex<_>>
  that means you're not really using the &mut self and could do with a &self

This commit closses issue: hyperium#3040
BREAKING CHANGE: The Service::call function no longer takes a mutable reference to self.
The FnMut trait bound on the service::util::service_fn function and the trait bound
on the impl for the ServiceFn struct were changed from FnMut to Fn
@rob2244 rob2244 force-pushed the refactor-service-trait branch from 60e9f84 to 4d90bc1 Compare May 12, 2023 20:28
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@seanmonstar
Copy link
Member

@programatik29 what's an adapater in hyper-util going to need to look like, do you think?

@rob2244 rob2244 closed this May 12, 2023
@rob2244 rob2244 reopened this May 12, 2023
@rob2244
Copy link
Contributor Author

rob2244 commented May 12, 2023

@programatik29 what's an adapater in hyper-util going to need to look like, do you think?

When you say adapter, are there structs in hyper-util that implement this trait? Couldn't we just change them there as well? Or am I missing something?

@programatik29
Copy link
Contributor

@seanmonstar With current tower services, I think our best bet is something like this:

https://github.com/davidpdrsn/tower-hyper-http-body-compat/blob/cbaa7df5246587082b770d91283daf646303c34e/src/service.rs#L31-L46

@seanmonstar
Copy link
Member

are there structs in hyper-util that implement this trait?

Not yet, but we could add them. Or there could be a separate crate, as was just linked. 🤷

@seanmonstar seanmonstar merged commit d894439 into hyperium:master May 15, 2023
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
change Service::call to take &self instead of &mut self.
Because of this change, the trait bound in the service::util::service_fn and
the trait bound in the impl for the ServiceFn struct were changed from FnMut to Fn.
This change was decided on for the following reasons:
- It prepares the way for async fn,
  since then the future only borrows &self, and thus a Service can concurrently handle
  multiple outstanding requests at once.
- It's clearer that Services can likely be cloned
- To share state across clones you generally need Arc<Mutex<_>>
  that means you're not really using the &mut self and could do with a &self

Closes hyperium#3040

BREAKING CHANGE: The Service::call function no longer takes a mutable reference to self.
  The FnMut trait bound on the service::util::service_fn function and the trait bound
  on the impl for the ServiceFn struct were changed from FnMut to Fn.
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
change Service::call to take &self instead of &mut self.
Because of this change, the trait bound in the service::util::service_fn and
the trait bound in the impl for the ServiceFn struct were changed from FnMut to Fn.
This change was decided on for the following reasons:
- It prepares the way for async fn,
  since then the future only borrows &self, and thus a Service can concurrently handle
  multiple outstanding requests at once.
- It's clearer that Services can likely be cloned
- To share state across clones you generally need Arc<Mutex<_>>
  that means you're not really using the &mut self and could do with a &self

Closes hyperium#3040

BREAKING CHANGE: The Service::call function no longer takes a mutable reference to self.
  The FnMut trait bound on the service::util::service_fn function and the trait bound
  on the impl for the ServiceFn struct were changed from FnMut to Fn.

Signed-off-by: Sven Pfennig <[email protected]>
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.

3 participants