DefaultListener::bindable() isn’t usable #2730

palant · 2024-02-19T11:36:07Z

Rocket Version

0.6.0 rev f75fb63

Operating System

Fedora Linux 39

Rust Toolchain Version

rustc 1.78.0-nightly (2bf78d12d 2024-02-18) and rustc 1.76.0 (07dca489a 2024-02-04)

What happened?

A low-level connection interface has been added in #1070 which I’m trying to use to implement more advanced TLS support. I’ve hit a roadblock however, as Rust won’t let me use the Bindable returned by DefaultListener::bindable(). I’ve been unable to find a work-around other than making DefaultListener::base_bindable() public or making DefaultListener::bindable() return Either<TlsBindable<std::net::SocketAddr>, TlsBindable<super::unix::UdsConfig>> explicitly instead of impl Binding. It is my understanding that the latter shouldn’t have any impact, but for some reason it does.

Note that having DefaultListener::base_bindable() public would be helpful regardless. Currently I have to remove TLS configuration from DefaultListener to make sure it won’t give me a TLS bindable.

Test Case

#[rocket::main]
async fn main() -> Result<(), rocket::Error> {
    let rocket = rocket::build();
    let config = rocket
        .figment()
        .extract::<rocket::listener::DefaultListener>()?;
    rocket.launch_on(config.bindable()?).await?;
    Ok(())
}

Log Output

error[E0277]: `impl std::future::Future<Output = Result<<impl Bindable as Bindable>::Listener, <impl Bindable as Bindable>::Error>>` cannot be sent between threads safely
   --> src/main.rs:2:46
    |
2   |   async fn main() -> Result<(), rocket::Error> {
    |  ______________________________________________^
3   | |     let rocket = rocket::build();
4   | |     let config = rocket.figment().extract::<rocket::listener::DefaultListener>()?;
5   | |     rocket.launch_on(config.bindable()?)
6   | |         .await?;
7   | |     Ok(())
8   | | }
    | | ^
    | | |
    | |_`impl std::future::Future<Output = Result<<impl Bindable as Bindable>::Listener, <impl Bindable as Bindable>::Error>>` cannot be sent between threads safely
    |   within this `{async block@src/main.rs:2:46: 8:2}`
    |
    = help: within `{async block@src/main.rs:2:46: 8:2}`, the trait `std::marker::Send` is not implemented for `impl std::future::Future<Output = Result<<impl Bindable as Bindable>::Listener, <impl Bindable as Bindable>::Error>>`, which is required by `{async block@src/main.rs:2:46: 8:2}: std::marker::Send`
note: `<impl Bindable as Bindable>::bind` is an `async fn` in trait, which does not automatically imply that its future is `std::marker::Send`
   --> Rocket/core/lib/src/listener/bindable.rs:10:5
    |
10  |     async fn bind(self) -> Result<Self::Listener, Self::Error>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: required because it captures the following types: `Rocket<Ignite>`, `impl std::future::Future<Output = Result<<impl Bindable as Bindable>::Listener, <impl Bindable as Bindable>::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/rocket.rs:690:90
    |
690 |       async fn _launch_on<B: Bindable>(self, bindable: B) -> Result<Rocket<Ignite>, Error> {
    |  __________________________________________________________________________________________^
691 | |         let listener = bindable.bind().await.map_err(|e| ErrorKind::Bind(Box::new(e)))?;
692 | |         self.serve(listener).await
693 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Build>`, `impl Bindable`, `rocket::phase::State`, `ControlFlow<Result<Infallible, rocket::Error>, Rocket<Ignite>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/rocket.rs:944:93
    |
944 |       pub async fn launch_on<B: Bindable>(self, bindable: B) -> Result<Rocket<Ignite>, Error> {
    |  _____________________________________________________________________________________________^
945 | |         match self.0.into_state() {
946 | |             State::Build(s) => Rocket::from(s).ignite().await?._launch_on(bindable).await,
947 | |             State::Ignite(s) => Rocket::from(s)._launch_on(bindable).await,
948 | |             State::Orbit(s) => Ok(Rocket::from(s).into_ignite())
949 | |         }
950 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Build>`, `DefaultListener`, `ControlFlow<Result<Infallible, rocket::Error>, impl Bindable>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` block
   --> src/main.rs:2:46
    |
2   |   async fn main() -> Result<(), rocket::Error> {
    |  ______________________________________________^
3   | |     let rocket = rocket::build();
4   | |     let config = rocket.figment().extract::<rocket::listener::DefaultListener>()?;
5   | |     rocket.launch_on(config.bindable()?)
6   | |         .await?;
7   | |     Ok(())
8   | | }
    | |_^
note: required by a bound in `async_main`
   --> Rocket/core/lib/src/lib.rs:252:66
    |
252 | pub fn async_main<R>(fut: impl std::future::Future<Output = R> + Send) -> R {
    |                                                                  ^^^^ required by this bound in `async_main`

error[E0277]: `impl std::future::Future<Output = Result<<<impl Bindable as Bindable>::Listener as Listener>::Accept, std::io::Error>>` cannot be sent between threads safely
   --> src/main.rs:2:46
    |
2   |   async fn main() -> Result<(), rocket::Error> {
    |  ______________________________________________^
3   | |     let rocket = rocket::build();
4   | |     let config = rocket.figment().extract::<rocket::listener::DefaultListener>()?;
5   | |     rocket.launch_on(config.bindable()?)
6   | |         .await?;
7   | |     Ok(())
8   | | }
    | | ^
    | | |
    | |_`impl std::future::Future<Output = Result<<<impl Bindable as Bindable>::Listener as Listener>::Accept, std::io::Error>>` cannot be sent between threads safely
    |   within this `{async block@src/main.rs:2:46: 8:2}`
    |
    = help: within `{async block@src/main.rs:2:46: 8:2}`, the trait `std::marker::Send` is not implemented for `impl std::future::Future<Output = Result<<<impl Bindable as Bindable>::Listener as Listener>::Accept, std::io::Error>>`, which is required by `{async block@src/main.rs:2:46: 8:2}: std::marker::Send`
note: `<<impl Bindable as Bindable>::Listener as Listener>::accept` is an `async fn` in trait, which does not automatically imply that its future is `std::marker::Send`
   --> Rocket/core/lib/src/listener/listener.rs:13:5
    |
13  |     async fn accept(&self) -> io::Result<Self::Accept>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: required because it captures the following types: `&listener::bounced::Bounced<<impl Bindable as Bindable>::Listener>`, `Result<<<impl Bindable as Bindable>::Listener as Listener>::Accept, std::io::Error>`, `impl std::future::Future<Output = Result<<<impl Bindable as Bindable>::Listener as Listener>::Accept, std::io::Error>>`, `std::io::Error`, `Sleep`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/listener/bounced.rs:28:67
    |
28  |       pub async fn accept_next(&self) -> <Self as Listener>::Accept {
    |  ___________________________________________________________________^
29  | |         loop {
30  | |             match self.listener.accept().await {
31  | |                 Ok(accept) => return accept,
...   |
38  | |         }
39  | |     }
    | |_____^
    = note: required because it captures the following types: `impl std::future::Future<Output = <listener::bounced::Bounced<<impl Bindable as Bindable>::Listener> as Listener>::Accept>`, `rocket::futures::future::Select<Pin<&mut impl std::future::Future<Output = <listener::bounced::Bounced<<impl Bindable as Bindable>::Listener> as Listener>::Accept>>, rocket::Shutdown>`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/listener/cancellable.rs:111:75
    |
111 |       pub async fn accept_next(&self) -> Option<<Self as Listener>::Accept> {
    |  ___________________________________________________________________________^
112 | |         let next = std::pin::pin!(self.listener.accept_next());
113 | |         match select(next, self.trigger.clone()).await {
114 | |             Either::Left((next, _)) => Some(next),
115 | |             Either::Right(_) => None,
116 | |         }
117 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Ignite>`, `hyper_util::server::conn::auto::Builder<hyper_util::rt::tokio::TokioExecutor>`, `std::time::Duration`, `listener::cancellable::CancellableListener<rocket::Shutdown, listener::bounced::Bounced<<impl Bindable as Bindable>::Listener>>`, `Arc<Rocket<Orbit>>`, `rocket::tokio::task::JoinHandle<()>`, `Arc<hyper_util::server::conn::auto::Builder<hyper_util::rt::tokio::TokioExecutor>>`, `Arc<listener::cancellable::CancellableListener<rocket::Shutdown, listener::bounced::Bounced<<impl Bindable as Bindable>::Listener>>>`, `impl std::future::Future<Output = std::option::Option<<listener::cancellable::CancellableListener<rocket::Shutdown, listener::bounced::Bounced<<impl Bindable as Bindable>::Listener>> as Listener>::Accept>>`, `std::array::IntoIter<std::time::Duration, 5>`, `Sleep`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/server.rs:89:5
    |
89  | /     {
90  | |         let mut builder = Builder::new(TokioExecutor::new());
91  | |         let keep_alive = Duration::from_secs(self.config.keep_alive.into());
92  | |         builder.http1()
...   |
178 | |         }
179 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Ignite>`, `impl std::future::Future<Output = Result<<impl Bindable as Bindable>::Listener, <impl Bindable as Bindable>::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/rocket.rs:690:90
    |
690 |       async fn _launch_on<B: Bindable>(self, bindable: B) -> Result<Rocket<Ignite>, Error> {
    |  __________________________________________________________________________________________^
691 | |         let listener = bindable.bind().await.map_err(|e| ErrorKind::Bind(Box::new(e)))?;
692 | |         self.serve(listener).await
693 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Build>`, `impl Bindable`, `rocket::phase::State`, `ControlFlow<Result<Infallible, rocket::Error>, Rocket<Ignite>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` fn body
   --> Rocket/core/lib/src/rocket.rs:944:93
    |
944 |       pub async fn launch_on<B: Bindable>(self, bindable: B) -> Result<Rocket<Ignite>, Error> {
    |  _____________________________________________________________________________________________^
945 | |         match self.0.into_state() {
946 | |             State::Build(s) => Rocket::from(s).ignite().await?._launch_on(bindable).await,
947 | |             State::Ignite(s) => Rocket::from(s)._launch_on(bindable).await,
948 | |             State::Orbit(s) => Ok(Rocket::from(s).into_ignite())
949 | |         }
950 | |     }
    | |_____^
    = note: required because it captures the following types: `Rocket<Build>`, `DefaultListener`, `ControlFlow<Result<Infallible, rocket::Error>, impl Bindable>`, `impl std::future::Future<Output = Result<Rocket<Ignite>, rocket::Error>>`
note: required because it's used within this `async` block
   --> src/main.rs:2:46
    |
2   |   async fn main() -> Result<(), rocket::Error> {
    |  ______________________________________________^
3   | |     let rocket = rocket::build();
4   | |     let config = rocket.figment().extract::<rocket::listener::DefaultListener>()?;
5   | |     rocket.launch_on(config.bindable()?)
6   | |         .await?;
7   | |     Ok(())
8   | | }
    | |_^
note: required by a bound in `async_main`
   --> Rocket/core/lib/src/lib.rs:252:66
    |
252 | pub fn async_main<R>(fut: impl std::future::Future<Output = R> + Send) -> R {
    |                                                                  ^^^^ required by this bound in `async_main`

For more information about this error, try `rustc --explain E0277`.

Additional Context

No response

System Checks

My bug report relates to functionality.
I have tested against the latest Rocket release or a recent git commit.
I have tested against the latest stable rustc toolchain.
I was unable to find this issue previously reported.

The text was updated successfully, but these errors were encountered:

SergioBenitez · 2024-02-19T12:51:56Z

This should definitely be improved, though I'll say that the intention behind the bindable() method isn't quite this use-case. Of course you wouldn't know that as the entire listener module is yet to be documented 🙃.

For your use-case in particular however, it sounds like you'd be better served defining your own structure with an EndPoint (and any other config, like TlsConfig) in it and creating your own bindable from that. Those parts are designed to be composable and reusable, whereas default listener is really only exposed to document the config parameters that Rocket uses for its listener by default. Given you mention actually wanting access to base_listener instead, which is mostly a simple cfg-aware match-and-extract, using your own config struct in your implementation will probably result in at least as clean an implementation as it otherwise would. And you get to avoid this particular shortcoming.

palant · 2024-02-19T13:02:12Z

I meant to replicate what the default TlsListener is doing – I don’t want to change the behavior, merely the configuration. So making assumptions about the default listener wasn’t my intention, I’ve already had to make way too many assumptions about Rocket internals. The real code is actually wrapping the default listener. But I guess this cannot be helped…

SergioBenitez · 2024-02-19T13:26:14Z

I meant to replicate what the default TlsListener is doing – I don’t want to change the behavior, merely the configuration.

I'm not sure how to reconcile this with your initial post. This would imply that your use-case would be achievable by:

Extracting the DefaultListener, as you did.
Tweaking the TlsConfig value.
Calling and listening on bindable().

And if this is the case, what would you gain from having access to base_bindable()?

So making assumptions about the default listener wasn’t my intention, I’ve already had to make way too many assumptions about Rocket internals.

Can you clarify? What assumptions are you having to make about either the default listener or any of Rocket's other internals? The listener module isn't documented yet, but you still shouldn't need to make any assumptions. The entire module is extremely type-directed. And the rest of Rocket is extensively documented.

The real code is actually wrapping the default listener. But I guess this cannot be helped…

Again, I'm really not sure what this means. It reads like a critique, but I'm not sure of what.

palant · 2024-02-19T14:13:27Z

Tweaking the TlsConfig value.

Nope, tweaking the ServerConfig from Rustls. It has SNI support whereas the default TlsConfig in Rocket doesn’t. So I have a custom config structure with per-host certificate configuration and TlsConfig as fallback.

What assumptions are you having to make about either the default listener or any of Rocket's other internals?

While I can deserialize TlsConfig as it is, I have to translate it into Rustls ServerConfig manually. So if this structure changes in future Rocket versions, my code might break.

And since I’m not reusing DefaultListener, I’ve also decided to limit my implementation to TCP sockets because supporting other socket types like UDS is considerable complexity and will likely break as Rocket evolves.

Quite frankly, all of this feels like I’m doing way too much work. It wouldn’t be necessary if Rocket allowed providing a custom rustls::server::ServerConfig instance like other frameworks do.

SergioBenitez · 2024-02-20T03:17:18Z

Tweaking the TlsConfig value.

Nope, tweaking the ServerConfig from Rustls. It has SNI support whereas the default TlsConfig in Rocket doesn’t. So I have a custom config structure with per-host certificate configuration and TlsConfig as fallback.

It wouldn't be very difficult to modify TlsConfig to add this support and upstream it into Rocket. That would make your work here much much easier while benefitting everyone else, too.

What assumptions are you having to make about either the default listener or any of Rocket's other internals?

While I can deserialize TlsConfig as it is, I have to translate it into Rustls ServerConfig manually. So if this structure changes in future Rocket versions, my code might break.

We internally have a function that converts the TlsConfig to a rustls config. If we exposed that, it sounds like this qualm would dissipate. Again, something you could choose to contribute that would benefit others while making your life much easier.

But to be clear, there's no assumption to be made here. The structure is well defined with a documented API. Obviously if there's a breaking change that impacts you, then you need to update your code. But that would be a choice you make. And that would happen with any library, in any language, and has nothing to do with Rocket's internals nor making assumptions about them.

And since I’m not reusing DefaultListener, I’ve also decided to limit my implementation to TCP sockets because supporting other socket types like UDS is considerable complexity and will likely break as Rocket evolves.

No, neither part of this statement is true. Listeners and connections compose, and since Rocket provides a UdsListener already, there's almost zero work you need to do to support them. If your custom TlsListener is designed generically, this would just work. There's no reason for you to assume that something will break as Rocket evolves.

Quite frankly, all of this feels like I’m doing way too much work. It wouldn’t be necessary if Rocket allowed providing a custom rustls::server::ServerConfig instance like other frameworks do.

I think you are doing too much work, but I don't think it's largely because Rocket isn't exposing the right primitives here, though obviously with my commentary above I concede that Rocket can make this easier.

We don't expose rustls directly because it's an implementation detail. We could easily switch to s2n tomorrow with 0 impact to users. That being said, as I've said before, I'm not opposed to exposing anything relevant here, as long as we do this in the right way.

Finally, I'd like to ask you to be more understanding about the feature-set you're using and criticizing: the entire thing landed just weeks ago and is still unreleased. To my knowledge, no other web framework in Rust offers the level of flexibility and composability that we've managed to get with the new listener/connection interfaces. Your commentary would be much better received if instead of only criticizing the project, you instead sought to improve it.

palant · 2024-02-20T06:41:17Z

Your commentary would be much better received if instead of only criticizing the project, you instead sought to improve it.

I’m not sure where you saw me criticizing the project. Let me assure you that I did attempt to solve this issue on my own, and I would have contributed a fix had I been successful. Unfortunately, my knowledge of Rust turned out to be insufficient.

Finally, I'd like to ask you to be more understanding about the feature-set you're using and criticizing: the entire thing landed just weeks ago and is still unreleased.

I am aware of that. That’s why I used a development version, with version 0.5 what I want cannot be done at all.

I am also aware of the primary use case for the new API. Still, it’s my assumption that there is value in trying out a new API and testing its limitations before it is released to everyone. Particularly things that look like they should work but don’t.

It wouldn't be very difficult to modify TlsConfig to add this support and upstream it into Rocket.

Ok, I can create a pull request. I think the best approach would be adding a new field to_rustls: Option<Fn(&Self, &Rocket) -> ServerConfig> to TlsConfig. Presumably, I can make it deserialize as None while still allowing to set tls.to_rustls in the figment programmatically.

There should also be a TlsConfig.default_to_rustls(&self) -> ServerConfig method to be used as fallback if the new field is None. It can also be called by custom implementations to provide an initial config which can then be modified. This would contain the code currently in TlsConfig::acceptor().

Obviously, documentation should containing a warning that Rustls (and its particular version) is an implementation detail which can change in future Rocket versions.

SergioBenitez · 2024-02-20T07:34:43Z

Ok, I can create a pull request. I think the best approach would be adding a new field to_rustls: Option<Fn(&Self, &Rocket) -> ServerConfig> to TlsConfig. Presumably, I can make it deserialize as None while still allowing to set tls.to_rustls in the figment programmatically.

I think all we need here are two things:

A TryFrom<TlsConfig> for rustls::ServerConfig implementation that simply calls the existing method (it's server_config(), currently on the h3 branch https://github.com/rwf2/Rocket/blob/h3/core/lib/src/listener/tls.rs)
A new crate feature, rustls or tls-rustls that this method is gates on.

Obviously, documentation should containing a warning that Rustls (and its particular version) is an implementation detail which can change in future Rocket versions.

A warning is insufficient. This needs to be a static guarantee, and a feature gate is the correct way to approach this.

I don't believe we need any additional functionality than this.

palant · 2024-02-20T09:19:47Z

How would a user provide a custom TlsConfig => ServerConfig conversion then? Even if it were possible to overwrite a trait implementation, Rust won’t let you write one (neither TlsConfig nor ServerConfig defined in current crate).

Note also that my suggestion takes the Rocket instance as input. That’s because a custom conversion might need additional configuration settings, so it needs access to Rocket::figment().

This commit introduces the ability to dynamically select a TLS configuration based on the client's TLS hello. Added `Authority::set_port()`. Various `Config` structures for listeners removed. `UdsListener` is now `UnixListener`. `Bindable` removed in favor of new `Bind`. `Connection` requires `AsyncRead + AsyncWrite` again The `Debug` impl for `Endpoint` displays the underlying address in plaintext. `Listener` must be `Sized`. `tls` listener moved to `tls::TlsListener` The preview `quic` listener no longer implements `Listener`. All built-in listeners now implement `Bind<&Rocket>`. Clarified docs for `mtls::Certificate` guard. No reexporitng rustls from `tls`. Added `TlsConfig::server_config()`. Added some future helpers: `race()` and `race_io()`. Fix an issue where the logger wouldn't respect a configuration during error printing. Added Rocket::launch_with(), launch_on(), bind_launch(). Added a default client.pem to the TLS example. Revamped the testbench. Added tests for TLS resolvers, MTLS, listener failure output. TODO: clippy. TODO: UDS testing. Resolves #2730. Resolves #2363. Closes #2748. Closes #2683. Closes #2577.

palant added the triage A bug report being investigated label Feb 19, 2024

SergioBenitez added deficiency Something doesn't work as well as it could and removed triage A bug report being investigated labels Feb 19, 2024

SergioBenitez mentioned this issue Mar 20, 2024

simpler impl of dynamically swapping tls creds #2748

Closed

SergioBenitez closed this as completed in 7cc818c Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DefaultListener::bindable() isn’t usable #2730

DefaultListener::bindable() isn’t usable #2730

palant commented Feb 19, 2024

SergioBenitez commented Feb 19, 2024

palant commented Feb 19, 2024

SergioBenitez commented Feb 19, 2024

palant commented Feb 19, 2024

SergioBenitez commented Feb 20, 2024 •

edited

palant commented Feb 20, 2024

SergioBenitez commented Feb 20, 2024

palant commented Feb 20, 2024 •

edited

DefaultListener::bindable() isn’t usable #2730

DefaultListener::bindable() isn’t usable #2730

Comments

palant commented Feb 19, 2024

Rocket Version

Operating System

Rust Toolchain Version

What happened?

Test Case

Log Output

Additional Context

System Checks

SergioBenitez commented Feb 19, 2024

palant commented Feb 19, 2024

SergioBenitez commented Feb 19, 2024

palant commented Feb 19, 2024

SergioBenitez commented Feb 20, 2024 • edited

palant commented Feb 20, 2024

SergioBenitez commented Feb 20, 2024

palant commented Feb 20, 2024 • edited

SergioBenitez commented Feb 20, 2024 •

edited

palant commented Feb 20, 2024 •

edited