-
Notifications
You must be signed in to change notification settings - Fork 0
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
[RFC] Change Service
associated Request
type to a type parameter
#1
Conversation
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
impl<D, I> WithPeakEwma<D, I> | ||
where | ||
D: Discover, | ||
I: Instrument<Handle, D::Response>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds on this impl block had to be removed because the Discover
trait was made generic, and the type parameter R
for Discover
's request type would be unbounded w/o PhantomData
.
S: Service, | ||
I: Instrument<Handle, S::Response>, | ||
{ | ||
impl<S, I> PeakEwma<S, I> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds on this impl block had to be removed because the R
type parameter on Service
would be unbounded without a PhantomData
field on PeakEwma
.
pub struct ResponseFuture<T> | ||
where T: Service, | ||
pub struct ResponseFuture<T, R> | ||
where T: Service<R>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional generic type parameters were added to most of the types in tower-buffer
for the request types of the wrapped Service
, rather than removing the T: Service
bounds. This was necessary since the associated types from the service type parameter are also used in struct definitions, so it was necessary to keep the T: Service
bound
struct ResponseInner<T, S> | ||
where S: Service, | ||
struct ResponseInner<T, S, R> | ||
where S: Service<R>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we need to maintain the S: Service
bound, since the struct definition uses S::Future
, so the additional request type parameter was added.
@@ -45,7 +44,7 @@ pub struct ResponseFuture<T> { | |||
#[derive(Debug)] | |||
enum State { | |||
// The service has hit its limit | |||
Limited(Sleep), | |||
Limited(Delay), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use the new tokio-timer
API as this crate no longer compiles otherwise. This is an unrelated change.
@@ -3,16 +3,18 @@ extern crate tower_mock; | |||
extern crate tower_rate_limit; | |||
extern crate tower_service; | |||
extern crate tokio_timer; | |||
extern crate tokio; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary to update these tests to use tokio
, as the new tokio_timer
wouldn't work otherwise. This is an unrelated change, but was necessary for the rate-limit
crate to compile.
pub struct ResponseFuture<T> | ||
where T: NewService | ||
pub struct ResponseFuture<T, R> | ||
where T: NewService<R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, the T: NewService
bound had to be maintained to use the T::Service
associated type, so an additional type parameter was added.
@@ -144,9 +142,28 @@ where T: NewService + fmt::Debug, | |||
} | |||
} | |||
|
|||
// Manual impl of Debug for State because we should be able to | |||
// format a state regardless of whether or not the request type | |||
// is debug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the State
type became generic over requests, we had to add this impl of Debug
as one could no longer be derived if R
is not Debug
, but R
is not a member of the enum.
inner: Option<T>, | ||
_req: PhantomData<fn() -> R>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A PhantomData
was necessary here because the impl of Future
for Ready
needs a T: Service
bound, and so we need the request type for that bound.
I think this could also be resolved by adding a T: Service<R>
bound on the definition of Ready
?
Closing this as the change has already been made upstream. |
As described in tower-rs#99, this branch changes the
Service::Request
associated type to a type parameter, and updates all thetower
crates to track that change.This PR is not necessarily intended to be merged, but is opened to allow the changes to be discussed and reviewed more easily.