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

util: add CloneBoxService #615

Merged
merged 4 commits into from
Nov 9, 2021
Merged

util: add CloneBoxService #615

merged 4 commits into from
Nov 9, 2021

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Nov 9, 2021

This upstreams a little utility I'm using a bunch in axum. Its often
useful to erase the type of a service while still being able to clone
it.

BoxService isn't Clone so previously you had to combine it with
Buffer but doing that a lot (which we did in axum) had measurable
impact on performance.

Note this doesn't add a !Send variant. I personally have no use
for that but if someone thinks it would be nice I can do a follow up PR.

@davidpdrsn davidpdrsn added C-enhancement Category: A PR with an enhancement or a proposed on in an issue. A-util Area: The tower "util" module labels Nov 9, 2021
@davidpdrsn davidpdrsn changed the title util: add CloneService util: add CloneBoxService Nov 9, 2021
This upstreams a little utility I'm using a bunch in axum. Its often
useful to erase the type of a service while still being able to clone
it.

`BoxService` isn't `Clone` previously you had to combine it with
`Buffer` but doing that a lot (which we did in axum) had measurable
impact on performance.
@davidpdrsn davidpdrsn force-pushed the david/clone-box-service branch from 9354ef2 to b291d66 Compare November 9, 2021 12:39
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Overall, this looks great to me --- we may also want to add methods to ServiceExt and/or ServiceBuilder for wrapping a service in CloneBoxService, but we can do that in a follow up PR.

I commented on a few minor things. The only one that I think is actually important to address before merging is the unnecessary fmt::Debug bounds on CloneBoxService's type parameters: since those types are never formatted in the Debug impl, it shouldn't be necessary for them to implement Debug.

Beyond that, this looks good to me and I'll be happy to approve it once that's addressed!

tower/src/util/clone_boxed.rs Outdated Show resolved Hide resolved
tower/src/util/clone_boxed.rs Show resolved Hide resolved
tower/src/util/clone_boxed.rs Outdated Show resolved Hide resolved
tower/src/util/clone_boxed.rs Outdated Show resolved Hide resolved
E: fmt::Debug,
{
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt.debug_struct("CloneBoxService").finish()
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: we may want to consider including the request and response type names here, so different instances of CloneBoxService can be distinguished? Maybe something like

Suggested change
fmt.debug_struct("CloneBoxService").finish()
write!(
fmt,
"CloneBoxService<Request = {}, Response = {}, Error = {}>",
std::any::type_name::<T>(),
std::any::type_name::<U>(),
std::any::type_name::<E>(),
)

but, on the other hand, this could be quite long, and we might want to correctly handle multi-line formatting in alt mode ({:#})...so, maybe that's something to hold off on and add later if it's needed...

Copy link
Member

Choose a reason for hiding this comment

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

also, BoxService doesn't do this, so it's probably better left for a separate PR if we do want to add something like that...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually thinking the type params are there such that this could be improved in the future, without a breaking change. I think I'll leave it as it is now and make an issue about improving the formatting, for BoxService as well.

Copy link
Member

Choose a reason for hiding this comment

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

I was actually thinking the type params are there such that this could be improved in the future,

I don't think we want the type params to be Debug in any case; we can't actually include values of those types in the formatted output, since they're not part of the struct. I think the where clause won't ever be useful for future improvements to the debug format, since we'll never have instances of those types to format when formatting a CloneBoxService instance. We can use type_name to get the names of those types without requiring those types to be Debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah doh of course! I've removed the bounds.

tower/CHANGELOG.md Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn requested a review from hawkw November 9, 2021 18:45
@davidpdrsn
Copy link
Member Author

@hawkw I think adding more convenience methods to ServiceExt and ServiceBuilder for this is a good idea. I'll do that in a follow up PR.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

okay, this looks good to me! thanks!

@davidpdrsn davidpdrsn merged commit 973bf71 into master Nov 9, 2021
@davidpdrsn davidpdrsn deleted the david/clone-box-service branch November 9, 2021 20:39
davidpdrsn added a commit that referenced this pull request Nov 18, 2021
Added

- **util**: Add `CloneBoxService` which is a `Clone + Send` boxed `Service` ([#615])
- **util**: Add `ServiceExt::boxed` and `ServiceExt::clone_boxed` for applying the
  `BoxService` and `CloneBoxService` middleware ([#616])
- **builder**: Add `ServiceBuilder::boxed` and `ServiceBuilder::clone_boxed` for
  applying `BoxService` and `CloneBoxService` layers ([#616])

Fixed

- **balance**: Remove redundant `Req: Clone` bound from `Clone` impls
  for `MakeBalance`, and `MakeBalanceLayer` ([#607])
- **balance**: Remove redundant `Req: Debug` bound from `Debug` impls
  for `MakeBalance`, `MakeFuture`, `Balance`, and `Pool` ([#607])
- **ready-cache**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `ReadyCache` ([#607])
- **steer**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `Steer` ([#607])
- **util**: Remove redundant `F: Clone` bound
  from `ServiceExt::map_request` ([#607])
- **docs**: Fix `doc(cfg(...))` attributes
  of `PeakEwmaDiscover`, and `PendingRequestsDiscover` ([#610])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for BoxService` ([#617])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for UnsyncBoxService` ([#617])

[#607]: #607
[#610]: #610
[#616]: #616
[#617]: #617
[#615]: #615
davidpdrsn added a commit that referenced this pull request Nov 18, 2021
Added

- **util**: Add `CloneBoxService` which is a `Clone + Send` boxed `Service` ([#615])
- **util**: Add `ServiceExt::boxed` and `ServiceExt::clone_boxed` for applying the
  `BoxService` and `CloneBoxService` middleware ([#616])
- **builder**: Add `ServiceBuilder::boxed` and `ServiceBuilder::clone_boxed` for
  applying `BoxService` and `CloneBoxService` layers ([#616])

Fixed

- **balance**: Remove redundant `Req: Clone` bound from `Clone` impls
  for `MakeBalance`, and `MakeBalanceLayer` ([#607])
- **balance**: Remove redundant `Req: Debug` bound from `Debug` impls
  for `MakeBalance`, `MakeFuture`, `Balance`, and `Pool` ([#607])
- **ready-cache**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `ReadyCache` ([#607])
- **steer**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `Steer` ([#607])
- **util**: Remove redundant `F: Clone` bound
  from `ServiceExt::map_request` ([#607])
- **docs**: Fix `doc(cfg(...))` attributes
  of `PeakEwmaDiscover`, and `PendingRequestsDiscover` ([#610])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for BoxService` ([#617])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for UnsyncBoxService` ([#617])

[#607]: #607
[#610]: #610
[#616]: #616
[#617]: #617
[#615]: #615
davidpdrsn added a commit that referenced this pull request Nov 18, 2021
* tower: prepare to release 0.4.11

Added

- **util**: Add `CloneBoxService` which is a `Clone + Send` boxed `Service` ([#615])
- **util**: Add `ServiceExt::boxed` and `ServiceExt::clone_boxed` for applying the
  `BoxService` and `CloneBoxService` middleware ([#616])
- **builder**: Add `ServiceBuilder::boxed` and `ServiceBuilder::clone_boxed` for
  applying `BoxService` and `CloneBoxService` layers ([#616])

Fixed

- **balance**: Remove redundant `Req: Clone` bound from `Clone` impls
  for `MakeBalance`, and `MakeBalanceLayer` ([#607])
- **balance**: Remove redundant `Req: Debug` bound from `Debug` impls
  for `MakeBalance`, `MakeFuture`, `Balance`, and `Pool` ([#607])
- **ready-cache**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `ReadyCache` ([#607])
- **steer**: Remove redundant `Req: Debug` bound from `Debug` impl
  for `Steer` ([#607])
- **util**: Remove redundant `F: Clone` bound
  from `ServiceExt::map_request` ([#607])
- **docs**: Fix `doc(cfg(...))` attributes
  of `PeakEwmaDiscover`, and `PendingRequestsDiscover` ([#610])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for BoxService` ([#617])
- **util**: Remove unnecessary `Debug` bounds from `impl Debug for UnsyncBoxService` ([#617])

[#607]: #607
[#610]: #610
[#616]: #616
[#617]: #617
[#615]: #615

* sorting

* Rename `CloneBoxService` to `BoxCloneService`

* formatting

* also update changelog
davidpdrsn added a commit to tokio-rs/axum that referenced this pull request Nov 18, 2021
Its been [upstreamed to tower](tower-rs/tower#615).
davidpdrsn added a commit to tokio-rs/axum that referenced this pull request Nov 18, 2021
Its been [upstreamed to tower](tower-rs/tower#615).
EdorianDark pushed a commit to EdorianDark/axum that referenced this pull request Nov 24, 2021
david-perez added a commit to smithy-lang/smithy-rs that referenced this pull request Nov 30, 2021
It has been [upstreamed to tower](tower-rs/tower#615) as
`BoxCloneService`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-util Area: The tower "util" module C-enhancement Category: A PR with an enhancement or a proposed on in an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants