Skip to content
Snippets Groups Projects

Minor retry tuning to improve behavior under failing conditions.

Merged Nick Mathewson requested to merge nickm/arti:retry_tuning_1 into main
1 unresolved thread

This branch fixes a couple of bugs and misdesigns that caused Arti to retry a few things too aggressively, or too many times. It fixes some of the "low hanging fruit" from #329 (closed).

Merge request reports

Approval is optional

Merged by Nick MathewsonNick Mathewson 2 years ago (Mar 31, 2022 12:37pm UTC)

Merge details

  • Changes merged into main with a461ddc9.
  • Deleted the source branch.
  • Auto-merge enabled

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
292 // This ensures that we always wait between attempts, but not after
293 // the final attempt.
294 if let Some(delay) = delay.take() {
295 debug!("Waiting {:?} for next download attempt...", delay);
296 futures::select_biased! {
297 _ = reset_timeout_future => {
298 info!("Download attempt timed out completely; resetting download state.");
299 state = state.reset()?;
300 continue 'next_state;
301 }
302 _ = FutureExt::fuse(runtime.sleep(delay)) => {}
303 };
304 }
305 // Make sure that `delay` is set for the next iteration of the loop.
306 delay = Some(retry.next_delay(&mut rand::thread_rng()));
307
  • Comment on lines +294 to +307

    This approach leaves delay in a wrong state in the body of the if let. In the (admittedly fairly unliikely) case that extra code appears there, it might leave delay set to None and continue or something. How about if let Some(delay) = mem::replace(&mut delay, ... next_delay ...), or maybe replacing delay with let mut skip_first_delay = iter::once(()) and then you can do if skip_first_delay.next().is_none() { let delay = ....

  • Please register or sign in to reply
  • Makes sense; I'm going with Option::replace as slightly better than mem::replace.

  • Nick Mathewson added 1 commit

    added 1 commit

    • c5e5fc15 - dirmgr: Use a different idiom in retry loop

    Compare with previous version

  • Nick Mathewson enabled an automatic merge when the pipeline for c5e5fc15 succeeds

    enabled an automatic merge when the pipeline for c5e5fc15 succeeds

  • Nick Mathewson mentioned in commit a461ddc9

    mentioned in commit a461ddc9

  • Please register or sign in to reply
    Loading