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

path to tower-service 1.0 #636

Open
2 tasks
hawkw opened this issue Feb 2, 2022 · 24 comments
Open
2 tasks

path to tower-service 1.0 #636

hawkw opened this issue Feb 2, 2022 · 24 comments
Assignees
Labels
A-service Area: The tower `Service` trait I-needs-decision Issues in need of decision. P-high High priority

Comments

@hawkw
Copy link
Member

hawkw commented Feb 2, 2022

Motivation

Tower has been used in production for over three years. Since the Service trait is intended to provide a common abstraction and compatibility interface across libraries, the definition of the Service trait is factored out into the tower-service crate so that it can remain stable even as other parts of the Tower ecosystem are changed. A significant number of major crates depend on tower-service, including hyper, tonic, warp, and axum. In most cases, these are public API dependencies. This means that these crates cannot currently release 1.0 versions while they expose tower-service in their public APIs.

The last time a breaking change was made to the Service trait was November 2019; this was in order to transition from futures 0.1 to std::future. While there have been a number of proposed improvements to the Service trait (e.g. #110, #136, #137, #319, #408, #412, #626), most of these have not ended up being practical, or require future language features that are not currently available on stable Rust.

Therefore, I think it's time to have a serious conversation about stabilizing the Service trait.

Proposal

In order to stabilize tower-service, I think we should do the following:

  1. Set an approximate time-frame for a stable release. If possible, coordinate with other libraries that have public API dependencies on tower-service and wish to release 1.0 versions (e.g. hyper).
  2. Thoroughly investigate proposed Service trait changes. Before stabilizing Service exactly as it exists right now, we should look into all the potential breaking changes that have been suggested, investigate whether they are possible, and assess the potential benefits of making them. This includes ideas like Service::disarm, tokens, splitting Service into PollReady and Call traits, etc. I think the best way to assess these ideas is to actually implement them in a branch. To really determine the benefits and drawbacks of these changes, it would probably also be good to fork a crate that uses tower-service, and try to update it to use the modified version. If this isn't practical, we should at least try to update a non-trivial example project; at the very least, we should be able to test out tower::balance and get the load-balancing example to work.
  3. Make any breaking changes. If any of the proposed breaking changes actually work and have noticeable benefits...we should make them, and release non-1.0 versions with that change, so that the ecosystem can try it out.
  4. Actually publish a 1.0 release. :)

Once we stabilize tower-service, we may want to consider the path to stability for tower-layer as well, but I think that's not as important.

Outstanding questions

@hawkw hawkw added A-service Area: The tower `Service` trait P-high High priority I-needs-decision Issues in need of decision. labels Feb 2, 2022
@hawkw
Copy link
Member Author

hawkw commented Feb 2, 2022

I went ahead and assigned everyone who might have opinions on this, hope that's not too annoying. :)

@hawkw
Copy link
Member Author

hawkw commented Feb 12, 2022

Quick ping — how do other maintainers (especially @seanmonstar, since it's potentially relevant to hyper 1.0) feel about this proposal?

@davidpdrsn
Copy link
Member

I think your proposal sounds good! I'm up for helping investigating the proposed changes and porting tower-http and axum to see how they work in practice.

One thing though: It all depends on the timeline that we agree on but I don't think we should necessarily wait for new features to stabilise in Rust. I think we should focus on coming up with a design that makes sense given what Rust looks like today, otherwise I'm afraid we'll never ship 😕

I also think we should consider a 1.0 of tower-layer (although less important). Personally I'm happy with it as is and not aware of any proposed changed.

tower-service being 1.0 would be huge for the ecosystem so I'm excited about thisk! axum isn't going 1.0 anytime soon but someday we'll have to think about it.

@LucioFranco
Copy link
Member

I think this makes a lot of sense. I believe most of the features we are waiting on are almost ready (and should be on nightly).

My main question though is we should 1.0 tower with hyper, so @seanmonstar how close is hyper to something like a 1.0?

@hawkw
Copy link
Member Author

hawkw commented Feb 14, 2022

@davidpdrsn

One thing though: It all depends on the timeline that we agree on but I don't think we should necessarily wait for new features to stabilise in Rust. I think we should focus on coming up with a design that makes sense given what Rust looks like today, otherwise I'm afraid we'll never ship confused

Yeah, I strongly agree with this. I think we should investigate the suggested changes that are potentially possible today, see if they work, and not worry about the Shiny Future too much. If there are significant language changes that impact the design of Service in the future, it would probably be fairly reasonable to consider a 2.0 release.

I also think we should consider a 1.0 of tower-layer (although less important). Personally I'm happy with it as is and not aware of any proposed changed.

Yeah, I agree; I left that out of this issue because it seemed out of scope. Fortunately, though, I think stabilizing tower-layer should be much more straightforward; I don't think there are any significant potential changes we might want to investigate. It just requires that the Service trait be stabilized, since it depends on tower-service.

@LucioFranco:

I think this makes a lot of sense. I believe most of the features we are waiting on are almost ready (and should be on nightly).

Hmm, what language features are you thinking about here? If you mean impl Trait in trait associated types, I think that may potentially make using the Service trait with async-await much nicer, but I don't think it significantly impacts the trait's design. Was there something else you had in mind?

My main question though is we should 1.0 tower with hyper, so @seanmonstar how close is hyper to something like a 1.0?

I think the point here is that if hyper 1.0 occurs before tower-service is stable, it will need to implement its own Service trait, rather than depend on tower-service. If we can stabilize tower-service before hyper releases its 1.0, it will probably be possible for hyper to continue to use Tower's Service trait. Stabilizing Service before hyper 1.0 seems like the ideal goal for us to aim for...but I'm not totally sure about Sean's planned timeline for that?

@LucioFranco
Copy link
Member

I'd like to add another note https://github.com/awslabs/smithy-rs/ is a pretty big consumer of tower so we should also consider their use.

Hmm, what language features are you thinking about here? If you mean impl Trait in trait associated types, I think that may potentially make using the Service trait with async-await much nicer, but I don't think it significantly impacts the trait's design. Was there something else you had in mind?

Yeah I am thinking GAT for the token style. So this would align with the splitting the ready and call part of the Service trait.

I think the point here is that if hyper 1.0 occurs before tower-service is stable, it will need to implement its own Service trait, rather than depend on tower-service. If we can stabilize tower-service before hyper releases its 1.0, it will probably be possible for hyper to continue to use Tower's Service trait. Stabilizing Service before hyper 1.0 seems like the ideal goal for us to aim for...but I'm not totally sure about Sean's planned timeline for that?

Our goal should be 100% to have hyper 1.0 with Service imo.

In addition, I think the big pain point with tower is poll_ready since this infects things like call requiring it to take &mut self. I think there are some improvements we can make ergonomically without changing the current service trait. If we provide ways for users to not have to interact with the Service trait directly via like mappers etc.

@seanmonstar
Copy link
Collaborator

how close is hyper to something like a 1.0?

This year. It's a team goal.

I think the point here is that if hyper 1.0 occurs before tower-service is stable, it will need to implement its own Service trait, rather than depend on tower-service

This is also true, and potentially still an option anyways. Whether hyper would come up with it's own trait, ditch the service pattern entirely for an accept/pull pattern, or anything else... hyper definitely couldn't depend on tower-service pre-1.0.

@LucioFranco
Copy link
Member

Right, my point is we should use hypers timeline to ensure that we 1.0 for inclusion into hypers 1.0 release. I think it would be pretty important ecosystem wise to accomplish that.

@davidpdrsn
Copy link
Member

Right, my point is we should use hypers timeline to ensure that we 1.0 for inclusion into hypers 1.0 release. I think it would be pretty important ecosystem wise to accomplish that.

I agree with that 👍

@davidpdrsn
Copy link
Member

I've filed #657 which I think we should at least consider as part of tower-service 1.0

@LegNeato
Copy link
Contributor

Apologize for my ignorance, but with GATs right around the corner, tower 1.0 will likely want to change the service trait ala https://tokio.rs/blog/2021-05-14-inventing-the-service-trait#gats, correct?

@davidpdrsn
Copy link
Member

GATs have been right around the corner for a long time 😅 I'm also not sure anyone has done a serious attempt at porting stuff to a GAT based service trait, yet at least.

@hawkw
Copy link
Member Author

hawkw commented Mar 28, 2022

Apologize for my ignorance, but with GATs right around the corner, tower 1.0 will likely want to change the service trait ala https://tokio.rs/blog/2021-05-14-inventing-the-service-trait#gats, correct?

Also, the specific note in the blog post you linked to suggests that GATs would allow us to remove the 'static bound on the Service::call future, by making it borrow the Service itself into the future. I don't actually think such a change is desirable even when it becomes possible.

Note that because the Service is mutably borrowed in call, allowing the returned future to borrow the Service would mean that the service cannot be called again while a call future exists. This would be very different from how Service currently works: any mutation of the Service itself is performed synchronously in the call method, and if the returned future needs to mutate shared state later, access to that shared state is synchronized by a Mutex, RwLock, channels, or other synchronization primitives. This allows multiple call futures to exist concurrently. Changing this would be a major change in how concurrent processing of requests would be implemented using tower, so I'm not sure if it's something we would do.

Also, note that futures will need to live for the 'static lifetime in order to be spawned, so in order to spawn a call future that mutably borrowed the Service, the service itself would need to be moved into the spawned future. So, again, the change described in the footnote would make implementing concurrent processing of requests much more difficult.

@hawkw
Copy link
Member Author

hawkw commented Mar 28, 2022

IMO, the main reason GATs are actually desirable for tower is because, if we wanted to implement a design with a Token type for storing readiness (e.g. #631), GATs would allow the Token to borrow the Service. However, it's not really clear if GATs would actually make that possible, either --- see #626 (comment)

@LucioFranco
Copy link
Member

Token types should allow call to take &self and would then allow us to migrate to async fn in traits, though I believe the required features for all of that is not around the corner like GATs is.

@o0Ignition0o
Copy link

o0Ignition0o commented Jun 2, 2022

It looks to me like the reason why #412 had been postponed is that the next batch of changes would happen when GATs stabilize.

Given it doesn't seem to be the case anymore, would now be the right time to have an other look at it, and try to get it in? Or do we feel like we still need GATs in order to "get disarm right" ?

@LegNeato
Copy link
Contributor

With GATs happening (rust-lang/rust#96709 (comment) ), it might be good to make some of the proposed changes to tower, at the very least to give feedback on GATs before final release?

@misterjoshua
Copy link

FYI GATs are now stable as of Rust 1.65.0.

https://blog.rust-lang.org/2022/11/03/Rust-1.65.0.html

@LucioFranco
Copy link
Member

FYI I am doing some work w/ async_fn in traits feature on nightly. Will eventually report back but since that feature is landing much sooner than I thought I think we should wait for that to land to 1.0 tower.

@leoyvens
Copy link
Contributor

I believe the lack of Send bounds would need to be resolved for async fn in traits to be suitable for tower.

@LucioFranco
Copy link
Member

@leoyvens correct, I've already communicated this to the async wg.

@ebkalderon
Copy link

I'd love to at least see a lifetime GAT added to the Future associated type, in the short term, so non-'static futures could be used in certain contexts. 😍 I do recognize this would be a pretty big breaking change, of course; all downstream services would have to add <'static> everywhere in order to use the new interface.

@rakshith-ravi

This comment was marked as off-topic.

@seanmonstar

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-service Area: The tower `Service` trait I-needs-decision Issues in need of decision. P-high High priority
Projects
None yet
Development

No branches or pull requests