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.

Sign up for GitHub

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

Jump to bottom

Change Service::call to take &self #3040

Closed
seanmonstar opened this issue Nov 1, 2022 · 5 comments
Closed

Change Service::call to take &self #3040

seanmonstar opened this issue Nov 1, 2022 · 5 comments
Labels
A-server Area: server. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Milestone
1.0 RC4

Comments

@seanmonstar
Copy link
Member

Now that hyper defines its own Service trait (#2853), there's a key part we should clarify before hitting 1.0 Final: Should it take &self or &mut self? Whichever the decision, the why should be documented (not necessarily in the API docs, but in code comments or in the docs/ folder).

The usual arguments are here:

  • &self
    • 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.
  • &mut self
    • If a Service has state (usually per-connection), it's cheaper to mutate it on each connection. Not being &mut would require internal synchronization.

I'm happy to open it up to discussion here. I'd recommend that comments are arguments be well-formed and reasoned, and try to provide something new, so that we keep the signal high. Chat is a fine place to bounce an idea around, and then come back and write it up so we can keep the history (decision history is extremely valuable).
@seanmonstar seanmonstar added A-server Area: server. B-rfc Blocked: More comments would be useful in determine next steps. B-breaking-change Blocked: this is an "API breaking change". labels Nov 1, 2022
@seanmonstar seanmonstar added this to the 1.0 Final milestone Nov 1, 2022
@davidpdrsn
Copy link
Member

My experience is that most services need to be Clone for two reasons:

  1. To get an owned reference that can be moved into the response future.
  2. When using tower::make::Shared (or similar)

Therefore 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. So while I haven't done much experimentation with writing &self services my gut feeling is that &self is better.

@LucioFranco
Copy link
Member

I will write up more once I have explored what the async fn in traits feature provides us. But from my experience tonic has used async fn handler(&self, Req) -> Res setup and had no issues. In fact, I think it expresses very well what the api does and allows users to add a mutex or atomic ops or static read only types etc. That said, we need this api to be future proof for what tower might be and does that mean we need to wait for GATs?

@asonix
Copy link

asonix commented Nov 2, 2022

This may have been brought up elsewhere, but actix-web's Service trait takes &self, but is otherwise very similar to Tower's trait

@fundon
Copy link
Contributor

fundon commented Nov 4, 2022

This is one of my practices Handler in Viz. &self should be sufficient for most scenarios.

@seanmonstar seanmonstar modified the milestones: 1.0 Final, 1.0 RC4 Feb 28, 2023
@seanmonstar seanmonstar added E-easy Effort: easy. A task that would be a great starting point for a new contributor. C-feature Category: feature. This is adding a new feature. and removed B-rfc Blocked: More comments would be useful in determine next steps. labels May 1, 2023
@seanmonstar seanmonstar changed the title Should Service::call take &self or &mut self? Change Service::call take &self May 1, 2023
@seanmonstar seanmonstar changed the title Change Service::call take &self Change Service::call to take &self May 1, 2023
@rob2244
Copy link
Contributor

rob2244 commented May 12, 2023

@seanmonstar you can assign this to me

rob2244 pushed a commit to rob2244/hyper that referenced this issue May 12, 2023
refactor(service): change Service::call to take &self
60e9f84 
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 mentioned this issue May 12, 2023
Merged
rob2244 pushed a commit to rob2244/hyper that referenced this issue May 12, 2023
refactor(service): change Service::call to take &self
4d90bc1 
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
mmastrac added a commit to denoland/deno that referenced this issue Jul 31, 2023
@mmastrac
feat(ext/http): Upgrade to hyper1.0-rc4 (#19987)
990ecc9 
Includes a lightly-modified version of hyper-util's `TokioIo` utility. 

Hyper changes:

v1.0.0-rc.4 (2023-07-10)
Bug Fixes

    http1:
http1 server graceful shutdown fix (#3261)
([f4b51300](hyperium/hyper@f4b5130))
send error on Incoming body when connection errors (#3256)
([52f19259](hyperium/hyper@52f1925),
closes hyperium/hyper#3253)
properly end chunked bodies when it was known to be empty (#3254)
([fec64cf0](hyperium/hyper@fec64cf),
closes hyperium/hyper#3252)

Features

client: Make clients able to use non-Send executor (#3184)
([d977f209](hyperium/hyper@d977f20),
closes hyperium/hyper#3017)
    rt:
replace IO traits with hyper::rt ones (#3230)
([f9f65b7a](hyperium/hyper@f9f65b7),
closes hyperium/hyper#3110)
add downcast on Sleep trait (#3125)
([d92d3917](hyperium/hyper@d92d391),
closes hyperium/hyper#3027)
service: change Service::call to take &self (#3223)
([d894439e](hyperium/hyper@d894439),
closes hyperium/hyper#3040)

Breaking Changes

Any IO transport type provided must not implement hyper::rt::{Read,
Write} instead of tokio::io traits. You can grab a helper type from
hyper-util to wrap Tokio types, or implement the traits yourself, if
it's a custom type.
([f9f65b7a](hyperium/hyper@f9f65b7))
client::conn::http2 types now use another generic for an Executor. Code
that names Connection needs to include the additional generic parameter.
([d977f209](hyperium/hyper@d977f20))
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 issue Jan 12, 2024
@rob2244 @0xE282B0
feat(service): change Service::call to take &self (hyperium#3223)
33b047b 
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 issue Jan 16, 2024
@rob2244 @0xE282B0
feat(service): change Service::call to take &self (hyperium#3223)
af59a65 
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]>
cratelyn added a commit to penumbra-zone/penumbra that referenced this issue Jan 24, 2024
@cratelyn
pd: 🔨 rework RootCommand::start auto-https logic
32b6457 
```
         /!\ this is a work in progress and will    /!\
         /!\ be force pushed until ready for review /!\
```

 ## 👀 overview

fixes #3627.

this reorganizes the logic in pd's startup code related to automatically
managed https functionality.

 ## 🎨 background & motivation

this PR, besides cleaning up the `rustls-acme`-related auto-https logic, is
also interested in *creating a state-of-affairs that will dovetail into
pr #3522*. in particular, this expression to start the GRPC serve given a
bound listener...

```rust
tokio::task::Builder::new()
    .name("grpc_server")
    .spawn(grpc_server.serve_with_incoming(tls_incoming))
    .expect("failed to spawn grpc server")
```

...should be adjusted so as to accept an `axum::Router`.

other tertiary requirements:
- when no `grpc_auto_https` flag has been configured, serve without using an ACME resolver
- ALPN permit http/1.1 for grpc-web backwards compatibility.

 ### ⚖️  `rustls-acme` and `tokio-rustls-acme`

quoth the #3627 description, citing an earlier comment:

> In the ~year since this code was written, there may be better options.
> `tokio-rustls-acme` seems promising
\- <#3522 (comment)>

for reference, the repositories for each live here, and here:
- <https://github.com/FlorianUekermann/rustls-acme>
- <https://github.com/n0-computer/tokio-rustls-acme>

after some comparison, i have come to the opinion that `rustls-acme` will still
be adequate for our purposes. the latter is a fork of the former, but active
development appears to have continued in the former, and i did not see any
particular "_must-have_" features for us in the latter.

 ## 🚰 dropping down to `axum`

(this part is lengthy...)

as stated above, we want to switch to an `axum::Router`. this means that
we won't be able to use the `AcmeConfig::incoming` function. the
`rustls-acme` library provides some "low-level" examples we can check
out:

- <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_tokio.rs>
- <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs>

`low_level_axum` sounds promising, given that #3522 itself drops down
from tonic into axum. a catch is that the "axum" example is referring to
`axum-server`, a library that wraps `axum`, rather than `axum` itself.
more on that momentarily.

we also use `tonic` 0.10.2 in pd, and elsewhere in the penumbra
monorepo. tonic isn't using hyper 1.x yet. this was being worked on in
hyperium/tonic#1583, continued on in hyperium/tonic#1595, and tracked in
hyperium/tonic#1579. that work also depends upon hyperium/hyper#3461.

this is complicated by the fact that `axum-server` 0.6.0 depends on
`hyper` 1.1.0. this is particularly relevant because of
hyperium/hyper#3040. so, we must be mindful that there is now a
`tower::Service` and a `hyper::Service`. `hyper-util` provides
facilities to convert between the two, and other helper glue.
additionally, see the "_upgrading"_ migration guide for more information
on the differences between pre- and post-1.0 interfaces in `hyper`.

- https://hyper.rs/guides/1/upgrading/
- <https://docs.rs/hyper-util/latest/hyper_util/service/struct.TowerToHyperService.html>

at a high level we need to take our `tonic::Router`, and turn that into
a `MakeService`, some object that can be used to _generate_ `Service`
instances to process a request.

this is additionally complicated by the fact that `axum-server` defines
its _own_ `MakeService` trait. that trait is sealed, blanket
implemented, and is a bound included in
`axum_server::server::Server::serve`.

```rust
// axum_server::serve
impl Server {
    pub async fn serve<M>(self, mut make_service: M) -> io::Result<()>
    where
        M: MakeService<SocketAddr, Request<Incoming>>,
        // ...
    { todo!() }
}
```

the `SocketAddr` parameter above is of importance.

`axum::Router::into_make_service` gives us `IntoMakeService<Router>`
- <https://docs.rs/axum/latest/axum/routing/struct.IntoMakeService.html>

```rust
impl<S, T> Service<T> for IntoMakeService<S>
where
    S: Clone,
{
    type Response = S;
    type Error = Infallible;
    type Future = IntoMakeServiceFuture<S>;
```

...which means that we are faced with this error:

```
error[E0277]: the trait bound `Router: tower_service::Service<http::request::Request<hyper::body::incoming::Incoming>>` is not satisfied
   --> crates/bin/pd/src/auto_https.rs:62:20
    |
62  |             .serve(make_svc)
    |              ----- ^^^^^^^^ the trait `tower_service::Service<http::request::Request<hyper::body::incoming::Incoming>>` is not implemented for `Router`
    |              |
    |              required by a bound introduced by this call
    |
    = help: the trait `tower_service::Service<axum::http::Request<axum::body::Body>>` is implemented for `Router`
    = help: for that trait implementation, expected `axum::http::Request<axum::body::Body>`, found `http::request::Request<hyper::body::incoming::Incoming>`
    = note: required for `IntoMakeService<Router>` to implement `axum_server::service::MakeService<std::net::SocketAddr, http::request::Request<hyper::body::incoming::Incoming>>`
```

`axum::Router::into_make_service` does not play well with
`axum_server::Server`.

...and if we specify the new `hyper::body::incoming::Incoming` as the
body type for the axum router,

```
error[E0599]: the method `into_make_service` exists for struct `Router<(), Incoming>`, but its trait bounds were not satisfied
  --> crates/bin/pd/src/auto_https.rs:57:31
   |
57 |         let make_svc = router.into_make_service();
   |                               ^^^^^^^^^^^^^^^^^ method cannot be called on `Router<(), Incoming>` due to unsatisfied trait bounds
   |
  ::: /home/katie/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-1.1.0/src/body/incoming.rs:47:1
   |
47 | pub struct Incoming {
   | ------------------- doesn't satisfy `hyper::body::Incoming: HttpBody`
   |
   = note: the following trait bounds were not satisfied:
           `hyper::body::Incoming: HttpBody`
```

`HttpBody` was the pre-1.0 trait, that is now `Body` post-1.0. in short, there
are some incongruencies at play as different libraries catch up to the new
breaking changes in `hyper`.

 ## 🤨 ...so now what?

TODO(kate): write up course of action for tomorrow.

---

🟥 NB: i worry about breakage due to untested load-bearing behavior
during a migration like this. see "tertiary requirements", above.
consider this a PR to be suspect of when deploying a new testnet.

Refs: #3627
Refs: #3646
Refs: #3522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-server Area: server. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
No open projects
hyper 1.0
Status: Done
Milestone
1.0 RC4
Development

No branches or pull requests
6 participants
@fundon @seanmonstar @davidpdrsn @asonix @LucioFranco @rob2244