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

Implement throttle combinator #736

Merged
merged 14 commits into from
Nov 19, 2018
Merged

Conversation

NeoLegends
Copy link
Contributor

@NeoLegends NeoLegends commented Nov 2, 2018

Second combinator implementing #732 (comment).

I'm not really sure on the design of this one (especially since errors of the underlying stream will be delayed as well, but I assume this is a necessary evil since you'd want to use this to slow down things like stream::repeat(_) that produce items at unbounded speeds), so I'm eager on your feedback.

You might notice I added two extensions to Stream, one that enforces a given delay, and another one that enforces a rate of items (and calculates the required delay from that). Not sure if both are necessary, but I felt they fit quite nicely.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Great start. I left some thoughts inline.

I'm OK w/ errors being delayed as well.

src/util/stream.rs Outdated Show resolved Hide resolved
tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Almost there 👍

tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
tokio-timer/src/throttle.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member

Could you merge master? This will hopefully fix the CI failure.

@NeoLegends
Copy link
Contributor Author

Rebased.

This adds get_ref, get_mut and into_inner accessors to Throttle. This is how the futures library does it as well.
@tobz
Copy link
Member

tobz commented Nov 18, 2018

@NeoLegends Can we also add a test or two for this one? Same story as the comment I left on the debounce combinator. :P

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

This looks great to me.

I'm happy to merge, but the PR still has [WIP] on it. Please confirm that you are ready to have this merged and I will do so.

@NeoLegends NeoLegends changed the title [WIP] Implement throttle combinator Implement throttle combinator Nov 19, 2018
@NeoLegends
Copy link
Contributor Author

Yes I am. :)

@carllerche
Copy link
Member

Great 👍 Thanks for sticking with it and getting this done. Looks great.

@carllerche carllerche merged commit e166c4d into tokio-rs:master Nov 19, 2018
@NeoLegends
Copy link
Contributor Author

My pleasure.

Please also see #747 :)

@NeoLegends NeoLegends deleted the feature/throttle branch November 20, 2018 10:32
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

3 participants