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

Switch from pin-project to pin-project-lite #594

Closed
seanmonstar opened this issue Jul 21, 2021 · 5 comments · Fixed by #637
Closed

Switch from pin-project to pin-project-lite #594

seanmonstar opened this issue Jul 21, 2021 · 5 comments · Fixed by #637
Labels
A-tower Area: the tower crate itself C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Milestone
0.5

Comments

@seanmonstar
Copy link
Collaborator

I doubt we actually need the extra features of the former, and changing to to "lite" will be a compilation speed improvement.
@seanmonstar seanmonstar added C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-help-wanted Call for participation: Help is requested to fix this issue. A-tower Area: the tower crate itself E-easy Call for participation: Experience needed to fix: Easy / not much labels Jul 21, 2021
@hawkw
Copy link
Member

hawkw commented Jul 21, 2021

Looks like tower::buffer currently uses PinnedDrop:

tower/tower/src/buffer/worker.rs

Lines 23 to 24 in e1760d3

#[pin_project(PinnedDrop)]
#[derive(Debug)]
which pin-project-lite doesn't support.

I think it may be possible to refactor the implementation somewhat so that the drop impl is not needed. We could probably add a wrapper struct around just the semaphore with a Drop impl, instead.

@Michael-J-Ward
Copy link
Contributor

Started the grind-work part of this migration.

Two limitations of pin-project-lite that I ran into:

  1. does not support tuple structs / variants - which is labelled as won't fix.
  2. Cannot interoperate with other field attributes including doc comments

This means each StateEnum needs a small refactor like this one.

@Michael-J-Ward Michael-J-Ward mentioned this issue Jul 24, 2021
Merged
2 tasks
@taiki-e
Copy link
Contributor

taiki-e commented Jul 27, 2021

which pin-project-lite doesn't support.

No. I recently added support for it: https://github.com/taiki-e/pin-project-lite/releases/tag/v0.2.7

@Michael-J-Ward
Copy link
Contributor

And implemented for buffer::Worker

@taiki-e is there anything preventing trait bounds on the PinnedDrop impl

@taiki-e
Copy link
Contributor

taiki-e commented Jul 27, 2021

ade48a2

However, it does not support generic trait bounds on the PinnedDrop impl.

To workaround this, I removed the T::Error bound from the Worker struct definition,
and moved close_semaphore to a a new impl without that trait bound.

Hmm, that's odd... I'll take a look later.

@Michael-J-Ward Michael-J-Ward mentioned this issue Jul 29, 2021
Merged
bors bot added a commit to taiki-e/pin-project-lite that referenced this issue Jul 30, 2021
@bors @Michael-J-Ward
Merge #64
ff1efb2 
64: Pinned drop bounds bug r=taiki-e a=Michael-J-Ward

The bug brought up in [this conversation](tower-rs/tower#594 (comment))

Simple fix, the parsing grammar wasn't handling trailing commas.

Co-authored-by: Michael J Ward <ward.michael.j@gmail.com>
@olix0r olix0r added this to the 0.5 milestone Jan 18, 2022
@davidpdrsn davidpdrsn mentioned this issue Feb 9, 2022
Merged
hawkw pushed a commit that referenced this issue Feb 17, 2022
@davidpdrsn
util: rework Either (#637)
522687c 
In practice I've found `Either` be hard to use since it changes the
error type to `BoxError`. That means if you combine two infallible
services you get a service that, to the type system, is fallible. That
doesn't work well with [axum's error
handling](https://docs.rs/axum/latest/axum/error_handling/index.html)
model which requires all services to be infallible and thus always
return a response. So you end up having to add boilerplate just to
please the type system.

Additionally, the fact that `Either` implements `Future` also means we
cannot fully remove the dependency on `pin-project` since
`pin-project-lite` doesn't support tuple enum variants, only named
fields.

This PR reworks `Either` to address these:

- It now requires the two services to have the same error type so no
  type information is lost. I did consider doing something like `where
  B::Error: From<A::Error>` but I hope this simpler model will lead to
  better compiler errors.
- Changes the response future to be a struct with a private enum using
  `pin-project-lite`
- Removes the `Future` impl so we can remove the dependency on
  `pin-project`

Goes without saying that this is a breaking change so we have to wait
until tower 0.5 to ship this.

cc @jplatte

Fixes #594
Fixes #550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tower Area: the tower crate itself C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Milestone
0.5
Development

Successfully merging a pull request may close this issue.

util: rework Either tower-rs/tower
5 participants
@seanmonstar @olix0r @hawkw @Michael-J-Ward @taiki-e