feat(channel): Make channel feature additive #1574

tottoto · 2023-11-19T00:27:23Z

Motivation

Allows users to use transport feature independent from channel.

Solution

Warning

This proposal includes a breaking change.

Makes channel feature additive.

djc

This looks interesting, but in its current form I find it hard to review. Would you be open to splitting this into smaller commits (or separate PRs if you prefer)? In particular, moving code around would be good to have in separate commits (at least).

djc · 2024-05-21T10:55:25Z

examples/Cargo.toml

@@ -298,13 +298,13 @@ hyper-warp-multiplex = ["hyper-warp"]
 uds = ["tokio-stream/net", "dep:tower", "dep:hyper"]
 streaming = ["tokio-stream", "dep:h2"]
 mock = ["tokio-stream", "dep:tower"]
-tower = ["dep:hyper", "dep:tower", "dep:http"]
+tower = ["dep:hyper", "dep:tower", "tower?/timeout", "dep:http"]


The ? isn't really needed here, right, since we already enable dep:tower unconditionally?

(Saw this also in some of your other PRs, so would be good to clarify the goal here.)

When I tested this previously, I found that it was necessary to write it like this at the Rust version which is older than or equal to 1.70. But when I tried it again this time, it seems that it is no longer necessary.

I am not sure why the results are different even though testing is done with the same Rust version.

ref:

Reduce hyper dependency tokio-rs/axum#2350 (comment)

Reduce hyper dependency tokio-rs/axum#2350 (comment)

I think I find what happen. At this example project, changing both of them to tower/feature removes all dep:tower notation, and then it provides implicit feature. However it is hidden by the explicit tower feature. There is no particular effect on the example, but to unify the notation, I will use ? in the notation.

I'm not sure I understand the full context of what you're saying. As far as I know, the ? means that the feature dependency should not (by itself) activate the optional dependency itself. That doesn't apply here (we're actively enabling the optional feature), so we should not use the ? here.

When using Rust 1.70 or older than the version, using dep:crate-name and crate-name/some-feature cannot be used at the same time because of the cargo limitation. dep:crate-name and crate-name?/some-feature is a workaround for the limitation. However, in this case, it seems to be overlooked by cargo due to the explicit feature which is the same name to the crate-name (tower in this case). Based on these premises, I prefer uniformity of notation.

So if we used 1.71 this limitation would be lifted? Sorry, I'm still not convinced.

Yes. When we bump MSRV to 1.71, this workaround is not needed.

Found the pull request which addressed this: rust-lang/cargo#12130.

This is just for examples though so I think this is fine for the moment.

djc · 2024-05-21T10:55:30Z

examples/Cargo.toml

 json-codec = ["dep:serde", "dep:serde_json", "dep:bytes"]
 compression = ["tonic/gzip"]
 tls = ["tonic/tls"]
 tls-rustls = ["dep:hyper", "dep:hyper-rustls", "dep:tower", "tower-http/util", "tower-http/add-extension", "dep:rustls-pemfile", "dep:tokio-rustls"]
 dynamic-load-balance = ["dep:tower"]
-timeout = ["tokio/time", "dep:tower"]
+timeout = ["tokio/time", "dep:tower", "tower?/timeout"]


djc · 2024-05-22T09:19:26Z

Allows users to use transport feature independent from channel.

Also, do you have a concrete use case in mind for this?

tottoto · 2024-05-22T11:48:55Z

Would you be open to splitting this into smaller commits (or separate PRs if you prefer)?

Sure, I will do it.

tottoto · 2024-05-22T12:43:44Z

Allows users to use transport feature independent from channel.

Also, do you have a concrete use case in mind for this?

Actually, the main purpose of this pull request is to prepare #1630, which split transport's server and channel completely.

However, this alone is useful for users who want to use it only as transport server without using channel, as it reduces additional dependencies. Currently, channel feature looks an independent feature from transport, but the dependencies and implementations are not separated, so users cannot use the channel functionality without transport.

djc · 2024-06-13T13:53:34Z

tonic/Cargo.toml

-  "dep:tower",
+  "dep:tower", "tower?/util", "tower?/limit",
+]
+channel = [


Can you update the crate root documentation to clarify a bit what the distinction is between channel and transport? I don't think the current docs are very clear:

transport: Enables the fully featured, batteries included client and server implementation based on hyper, tower and tokio. Enabled by default.
channel: Enables just the full featured channel/client portion of the transport feature.

Updated to reflect the state of these feature flags.

djc · 2024-06-13T13:54:42Z

examples/Cargo.toml

@@ -276,13 +276,13 @@ tracing = ["dep:tracing", "dep:tracing-subscriber"]
 uds = ["tokio-stream/net", "dep:tower", "dep:hyper", "dep:hyper-util"]
 streaming = ["tokio-stream", "dep:h2"]
 mock = ["tokio-stream", "dep:tower", "dep:hyper-util"]
-tower = ["dep:hyper", "dep:hyper-util", "dep:tower", "dep:http"]
+tower = ["dep:hyper", "dep:hyper-util", "dep:tower", "tower?/timeout", "dep:http"]


Does the previous commit fail to compile the examples? If so, IMO we should squash this commit into it.

Does the previous commit fail to compile the examples?

Yes. Squashed these two commits.

djc · 2024-06-13T13:56:11Z

tonic/src/transport/service/discover.rs

+use tokio_stream::Stream;
+use tower::discover::Change;
+
+type DiscoverResult<K, S, E> = Result<Change<K, S>, E>;


This seems unrelated to the commit message?

tottoto · 2024-06-15T01:57:39Z

Addressed the reviews, and updated the feature configs.

LucioFranco

LGTM, looks like we need to get #1630 updated as well.

tottoto force-pushed the make-channel-feature-additive branch 2 times, most recently from 633ffb2 to 32dbd14 Compare January 5, 2024 22:00

tottoto force-pushed the make-channel-feature-additive branch 6 times, most recently from a8d667a to 1fe8cb0 Compare February 8, 2024 22:54

tottoto mentioned this pull request Feb 9, 2024

feat(transport): Make transport server and channel independent #1630

Merged

tottoto force-pushed the make-channel-feature-additive branch from 1fe8cb0 to 4d0d3ea Compare February 21, 2024 11:56

tottoto force-pushed the make-channel-feature-additive branch from 4d0d3ea to 2f5ab49 Compare March 1, 2024 10:40

tottoto force-pushed the make-channel-feature-additive branch 5 times, most recently from 44071c7 to bfdac3b Compare April 30, 2024 13:29

tottoto force-pushed the make-channel-feature-additive branch from bfdac3b to 4c3efe5 Compare May 21, 2024 10:26

djc reviewed May 21, 2024

View reviewed changes

tottoto marked this pull request as draft May 22, 2024 12:37

tottoto force-pushed the make-channel-feature-additive branch from 4c3efe5 to f455436 Compare May 22, 2024 12:47

djc added the breaking change label May 23, 2024

tottoto force-pushed the make-channel-feature-additive branch 2 times, most recently from cec6c48 to b48685e Compare May 23, 2024 22:27

tottoto marked this pull request as ready for review May 23, 2024 22:35

tottoto requested a review from djc May 23, 2024 22:35

tottoto force-pushed the make-channel-feature-additive branch from b48685e to e409efb Compare May 26, 2024 11:18

tottoto mentioned this pull request May 28, 2024

Upgrade to Hyper 1.0 & Axum 0.7 #1670

Merged

17 tasks

djc reviewed Jun 13, 2024

View reviewed changes

tottoto force-pushed the make-channel-feature-additive branch 5 times, most recently from 6849880 to 0e9cf89 Compare June 15, 2024 01:50

tottoto force-pushed the make-channel-feature-additive branch from 0e9cf89 to 3f3acd4 Compare June 15, 2024 09:06

tottoto added 14 commits June 15, 2024 21:39

feat(channel): Make channel feature additive

b2968bc

chore(channel): Move add_origin service module to channel module

418725e

chore(channel): Move user_agent service to channel module

9b21895

chore(channel): Move reconnect service to channel module

a1a9f37

chore(channel): Move connection service to channel module

02b3d11

chore(channel): Move discover service to channel module

48b009b

feat(channel): Remove unused Connected implement for BoxedIo

1cc3f9f

chore(channel): Move channel part of io service to channel module

b9ed61b

chore(channel): Move channel part of tls service to channel module

3d54b57

chore(channel): Move connector service to channel module

e773590

chore(channel): Move executor service to channel module

cf8e302

chore(channel): Clean up scope of channel service module

76128a4

chore(channel): Clean up importing in channel service

2724a96

chore(doc): Update feature flag document about transport and channel

3b54bae

tottoto force-pushed the make-channel-feature-additive branch from 3f3acd4 to 3b54bae Compare June 15, 2024 12:46

LucioFranco approved these changes Jun 19, 2024

View reviewed changes

LucioFranco added this pull request to the merge queue Jun 19, 2024

Merged via the queue into hyperium:master with commit b947e1a Jun 19, 2024
16 checks passed

tottoto deleted the make-channel-feature-additive branch June 19, 2024 22:00

LucioFranco mentioned this pull request Jun 20, 2024

chore: release v0.12.0 #1740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(channel): Make channel feature additive #1574

feat(channel): Make channel feature additive #1574

tottoto commented Nov 19, 2023

djc left a comment

djc May 21, 2024

djc May 22, 2024

tottoto May 22, 2024

tottoto May 22, 2024

djc May 23, 2024

tottoto May 23, 2024

djc Jun 13, 2024

tottoto Jun 13, 2024

LucioFranco Jun 19, 2024

djc May 21, 2024

djc commented May 22, 2024

tottoto commented May 22, 2024

tottoto commented May 22, 2024

djc Jun 13, 2024

tottoto Jun 13, 2024

djc Jun 13, 2024

tottoto Jun 13, 2024

djc Jun 13, 2024

tottoto Jun 13, 2024

tottoto commented Jun 15, 2024

LucioFranco left a comment

feat(channel): Make channel feature additive #1574

feat(channel): Make channel feature additive #1574

Conversation

tottoto commented Nov 19, 2023

Motivation

Solution

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc commented May 22, 2024

tottoto commented May 22, 2024

tottoto commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tottoto commented Jun 15, 2024

LucioFranco left a comment

Choose a reason for hiding this comment