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

Remove Boxing of response futures #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove Boxing of response futures #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
346858d
Remove Boxing
jonhoo Sep 12, 2019
5ad387a
Work around https://github.com/rust-lang/rust/issues/65863
jonhoo Jun 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ where

/// Attempted to issue a `call` when no more requests can be in flight.
///
/// See [`tower_service::Service::poll_ready`] and [`Client::with_limit`].
/// See [`tower_service::Service::poll_ready`].
TransportFull,

/// Attempted to issue a `call`, but the underlying transport has been closed.
Expand Down
18 changes: 18 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
rust_2018_idioms
)]
#![allow(clippy::type_complexity)]
#![feature(type_alias_impl_trait)]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you can feature flag this, a bit unfortunate since this doesn't seem like its going to hit stable anytime soon :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really want this PR to land, but yeah, not looking to stabilize for a while. It would essentially be an unstable feature flag, which isn't great.. :/

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you could do this with some sort of generic or something maybe but yeah, go head we can fix later but we should probably not get things stuck on the nightly drug. I just asked in zulip about this feature not gonna be anytime soon :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So you're proposing we fold this under, like feature = "nightly-nobox"? I don't think that'll work, because then the feature wouldn't be additive. If something that depends on tokio-tower without the feature somehow got compiled with the feature, it might not compile.

Copy link
Member

Choose a reason for hiding this comment

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

yeah im not sure but happy to not block this now for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think putting a version out there with non-additive features is just asking for downstream users to have issues sadly. I think it's fine to keep the Box version for now, and then have an explicit git opt-in for those (like me) who want to squeeze the last little bit out.


const YIELD_EVERY: usize = 24;

Expand Down Expand Up @@ -231,3 +232,20 @@ mod sealed {

pub mod multiplex;
pub mod pipeline;

/// impl Future.
///
/// https://github.com/rust-lang/rust/issues/65863
#[cfg(doc)]
pub struct DocFuture<O>(std::marker::PhantomData<O>);

#[cfg(doc)]
impl<O> std::future::Future for DocFuture<O> {
type Output = O;
fn poll(
self: std::pin::Pin<&mut Self>,
_: &mut std::task::Context<'_>,
) -> std::task::Poll<Self::Output> {
unreachable!()
}
}
18 changes: 13 additions & 5 deletions src/multiplex/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,15 @@ where
{
type Error = SpawnError<NT::MakeError>;
type Response = Client<NT::Transport, Error<NT::Transport, Request>, Request>;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

#[cfg(not(doc))]
type Future = impl Future<Output = Result<Self::Response, Self::Error>> + Send;
#[cfg(doc)]
type Future = crate::DocFuture<Result<Self::Response, Self::Error>>;

fn call(&mut self, target: Target) -> Self::Future {
let maker = self.t_maker.make_transport(target);
Box::pin(async move { Ok(Client::new(maker.await.map_err(SpawnError::Inner)?)) })
async move { Ok(Client::new(maker.await.map_err(SpawnError::Inner)?)) }
}

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
Expand Down Expand Up @@ -390,7 +394,11 @@ where
{
type Response = T::Ok;
type Error = E;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

#[cfg(not(doc))]
type Future = impl Future<Output = Result<Self::Response, Self::Error>> + Send;
#[cfg(doc)]
type Future = crate::DocFuture<Result<Self::Response, Self::Error>>;

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), E>> {
Poll::Ready(ready!(self.mediator.poll_ready(cx)).map_err(|_| E::from(Error::ClientDropped)))
Expand All @@ -402,7 +410,7 @@ where
tracing::trace!("issuing request");
let req = ClientRequest { req, span, res: tx };
let r = self.mediator.try_send(req);
Box::pin(async move {
async move {
match r {
Ok(()) => match rx.await {
Ok(r) => {
Expand All @@ -413,7 +421,7 @@ where
},
Err(_) => Err(E::from(Error::TransportFull)),
}
})
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/multiplex/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub mod server;
pub use self::server::Server;

/// A convenience wrapper that lets you take separate transport and tag store types and use them as
/// a single [`client::Transport`].
/// a single transport.
#[pin_project]
#[derive(Debug)]
pub struct MultiplexTransport<T, S> {
Expand Down
18 changes: 13 additions & 5 deletions src/pipeline/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,15 @@ where
{
type Error = SpawnError<NT::MakeError>;
type Response = Client<NT::Transport, Error<NT::Transport, Request>, Request>;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

#[cfg(not(doc))]
type Future = impl Future<Output = Result<Self::Response, Self::Error>> + Send;
#[cfg(doc)]
type Future = crate::DocFuture<Result<Self::Response, Self::Error>>;

fn call(&mut self, target: Target) -> Self::Future {
let maker = self.t_maker.make_transport(target);
Box::pin(async move { Ok(Client::new(maker.await.map_err(SpawnError::Inner)?)) })
async move { Ok(Client::new(maker.await.map_err(SpawnError::Inner)?)) }
}

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
Expand Down Expand Up @@ -350,7 +354,11 @@ where
{
type Response = T::Ok;
type Error = E;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

#[cfg(not(doc))]
type Future = impl Future<Output = Result<Self::Response, Self::Error>> + Send;
#[cfg(doc)]
type Future = crate::DocFuture<Result<Self::Response, Self::Error>>;

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), E>> {
Poll::Ready(ready!(self.mediator.poll_ready(cx)).map_err(|_| E::from(Error::ClientDropped)))
Expand All @@ -362,7 +370,7 @@ where
tracing::trace!("issuing request");
let req = ClientRequest { req, span, res: tx };
let r = self.mediator.try_send(req);
Box::pin(async move {
async move {
match r {
Ok(()) => match rx.await {
Ok(r) => {
Expand All @@ -373,7 +381,7 @@ where
},
Err(_) => Err(E::from(Error::TransportFull)),
}
})
}
}
}

Expand Down