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

makeBackoffMachine: Improve backoff sleep logic by making it local. #3841

Merged
merged 6 commits into from
Apr 10, 2020

Commits on Mar 4, 2020

  1. eslint: Disable no-await-in-loop.

    The docs at https://eslint.org/docs/rules/no-await-in-loop say,
    "Usually, the code should be refactored to create all the promises
    then get access to the results using `Promise.all()`. Otherwise, each
    successive operation will not start until the previous one has
    completed."
    
    But sometimes invoking one asynchronous task depends on the
    resolution of the previous task, or it's important to have only one
    task in progress at a time. If either of these is important, the
    programmer must use an `eslint-disable` within their `for` or
    `while` loop.
    
    The only other choice that seems reasonable is to use
    Array.prototype.forEach, but this will introduce a bug: A promise
    returned by the callback passed to forEach is not `await`ed before
    the next invocation of the callback.
    
    ['a', 'b', 'c'].forEach(async letter => {
      await new Promise(resolve => setTimeout(resolve, 1000));
      console.log(letter);
    });
    
    The desired result is
    
    // (one second passes)
    // 'a'
    // (one second passes)
    // 'b'
    // (one second passes)
    // 'c'
    
    but the actual result is
    
    // (one second passes)
    // 'a'
    // 'b'
    // 'c'
    
    With this rule, if you want to `await` sequentially in a loop, you
    have to choose between an `eslint-disable` and the failure of using
    a forEach. It's reasonable to assume that someone will choose wrong
    because they trust our choice of rules and they're not looking
    closely at the implementation of forEach. So, we're better without
    it.
    Chris Bobbe committed Mar 4, 2020
    Configuration menu
    Copy the full SHA
    c36e2bb View commit details
    Browse the repository at this point in the history
  2. backoff machine: Add, for replacement of progressiveTimeout.

    Improve state logic for sleep durations in API request retry loops.
    
    progressiveTimeout uses a global state, with the effect that one
    network request's retry attempts affect the sleep durations used for
    any other request. This has the benefit of enabling a general
    throttle on all request retry loops that used progressiveTimeout.
    But different requests may have transient failures for different
    reasons, so it's not obvious that we want this general throttle. And
    hard-to-trace bugs can creep in when the behavior of
    progressiveTimeout can't be determined from a particular call site.
    
    Also, progressiveTimeout uses a 60-second threshold as a heuristic
    to distinguish request retry loops from each other, with the
    simplifying assumption that different types of requests will not
    occur within 60 seconds of each other. This distinction is more
    effectively done by managing the state per-loop in the first place,
    and doing so eliminates more of those hard-to-trace bugs mentioned
    in the previous paragraph.
    
    So, introduce the ability to handle state locally. Capped
    exponential backoff still mitigates high request traffic, but
    per-request.
    
    Preparation for zulip#3829, to be completed in this series of commits.
    Chris Bobbe committed Mar 4, 2020
    Configuration menu
    Copy the full SHA
    577ac4c View commit details
    Browse the repository at this point in the history
  3. backoff machine: Add jitter.

    The benefits of "jitter" are large, and the implementation is not
    complex. With plain capped exponential backoff, if there is a burst
    of client requests, they will all back off together, resulting in
    more bursts, with gaps in between. Introducing randomness,
    particularly a lot of randomness, smooths out those bursts.
    
    Greg refers to this article:
    https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/
    and he discusses the advantages of "jitter" in more detail at
    https://chat.zulip.org/#narrow/stream/92-learning/topic/exponential.20backoff.20--.20with.20jitter.
    There, he claims that the work to be done with "full jitter" is O(N log N),
    a great improvement over O(N^2) without any jitter.
    Chris Bobbe committed Mar 4, 2020
    Configuration menu
    Copy the full SHA
    a301dda View commit details
    Browse the repository at this point in the history
  4. tryUntilSuccessful [nfc]: Make iterative, not recursive.

    Enable the use of makeBackoffMachine in tryUntilSuccessful. We need
    to call `new BackoffMachine()` in the body of tryUntilSuccessful,
    but above the loop. This isn't possible when tryUntilSuccessful is
    recursive.
    
    Now we can call `new BackoffMachine()` inside the function but above the
    loop.
    Chris Bobbe committed Mar 4, 2020
    Configuration menu
    Copy the full SHA
    eb20721 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    9c1131f View commit details
    Browse the repository at this point in the history
  6. progressiveTimeout: Delete; was entirely replaced by new BackoffMachi…

    …ne().
    
    Fixes: 3840
    Chris Bobbe committed Mar 4, 2020
    Configuration menu
    Copy the full SHA
    a3a442d View commit details
    Browse the repository at this point in the history