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

fix(transport): Make server builder more consitient #901

Merged
merged 2 commits into from Feb 18, 2022

Conversation

jdahlq
Copy link
Contributor

@jdahlq jdahlq commented Jan 30, 2022

Motivation

See Issue #900

Solution

See Issue #900

@jdahlq jdahlq changed the title tonic: fix consistency issue with timeout func in transport/server bu… tonic: fix consistency issue with timeout func in server builder Jan 30, 2022
@jdahlq
Copy link
Contributor Author

jdahlq commented Feb 12, 2022

I guess this might cause a silent breakage for code like this:

let router = Server::builder();
router.timeout(Duration::from_secs(10));

router.timeout would no longer do anything if not chained or reassigned. We could add a #[must_use] annotation (and probably should add it for all these functions which return Self), which presumably would cause a compiler error for users with such code. Not sure what the best course of action is, here.

@LucioFranco
Copy link
Member

Yeah I think adding must use makes sense here then. If you want to go ahead and add that change here as well we can merge this.

@jdahlq
Copy link
Contributor Author

jdahlq commented Feb 16, 2022

Done.

@LucioFranco
Copy link
Member

Can you rebase against master? I fixed the ci failure #914

…ilder

transport/server/mod.rs exposes a Builder for the Server, and most of the
Builder functions return Self (a change made for issue hyperium#115), but timeout
returns mut& Self. This change makes timeout consistent with the rest of
the Builder api by returning Self.

Fixes: hyperium#900
Refs: hyperium#115
@jdahlq
Copy link
Contributor Author

jdahlq commented Feb 18, 2022

Thanks, rebased.

@LucioFranco LucioFranco changed the title tonic: fix consistency issue with timeout func in server builder fix(transport): Make server builder more consitient Feb 18, 2022
@LucioFranco LucioFranco merged commit 6763d19 into hyperium:master Feb 18, 2022
@jdahlq jdahlq deleted the tonic-transport-timeout-fix branch February 19, 2022 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants