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

throttle function has bigger delays than it should #820

Closed
dcorb opened this Issue Oct 9, 2012 · 4 comments

Comments

Projects
None yet
3 participants
@dcorb

dcorb commented Oct 9, 2012

I found this minor bug when trying to make a demo to visualize throttle and debounce:

http://drupalmotion.com/article/debounce-and-throttle-visual-explanation

In certain cases, it will skip a execution and waits more (double wait time) than necessary.

I tracked it down to this scenario:

When executing a throttle'd function, there are 2 setTimeouts executed almost simultaneously
with exactly the same delay, AND in this execution order:

  • One for later()
  • One for whenDone() (internally in the _.debounce )

The problem is that later() will trigger a whenDone() also (aka, that same _.debounce)

Then, it can happen two things with that second _.debounce:

  • The first _debounce has finished. [Idealistic]
  • The first _debounce hasn't finished [Problematic], so the second _debounce will clearTimeout the first one.

I continue the bug report with an example in this text file that doesn't break the format:

http://drupalmotion.com/sites/default/files/demos/throttle.txt

@jashkenas jashkenas closed this in bb97a4d Oct 9, 2012

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Oct 9, 2012

Owner

Thanks for the report. I've updated the function to use effectively the same implementation as Lodash, removing the two potential timeouts.

Owner

jashkenas commented Oct 9, 2012

Thanks for the report. I've updated the function to use effectively the same implementation as Lodash, removing the two potential timeouts.

@dcorb

This comment has been minimized.

Show comment
Hide comment
@dcorb

dcorb Oct 9, 2012

Cool !. I updated the blog post to clarify that has been fixed in the master branch.

dcorb commented Oct 9, 2012

Cool !. I updated the blog post to clarify that has been fixed in the master branch.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Oct 9, 2012

Contributor

The updated throttle implementation also resolves issue #502 and helps stabilize existing throttle unit tests (the remaining calculation helps with timeout drift causing false test failings).

Don't forget unit tests (here and here).

Contributor

jdalton commented Oct 9, 2012

The updated throttle implementation also resolves issue #502 and helps stabilize existing throttle unit tests (the remaining calculation helps with timeout drift causing false test failings).

Don't forget unit tests (here and here).

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Oct 10, 2012

Owner

Don't forget unit tests (here and here)

Hey, go for it. I've given you push access to the repo.

Owner

jashkenas commented Oct 10, 2012

Don't forget unit tests (here and here)

Hey, go for it. I've given you push access to the repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment