Opened 7 months ago

Closed 7 months ago

#20593 closed defect (fixed)

Avoid resetting download status on 503

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: CoreTorTeam201611
Cc: Actual Points:
Parent ID: #20499 Points:
Reviewer: Sponsor:

Description

On comment:31:ticket:20499 , Teor notes:

If we really do want every failure to result in a schedule increment, we have to remove the following code:

download_status_increment_failure:

  /* only count the failure if it's permanent, or we're a server */
  if (status_code != 503 || server) {
    if (dls->n_download_failures < IMPOSSIBLE_TO_DOWNLOAD-1)
      ++dls->n_download_failures;
  }

Because combined with this code in download_status_schedule_get_delay, it causes a reset on every 503:

    /* Check if we missed a reset somehow */
    if (dls->last_backoff_position > dls_schedule_position) {
      dls->last_backoff_position = 0;
      dls->last_delay_used = 0;
    }

Which is exactly what we don't want when relays are busy - imagine clients doing an automatic reset every time they DoS a relay...

Child Tickets

Change History (4)

comment:1 Changed 7 months ago by nickm

  • Status changed from new to needs_review

bug20534_029 is my attempt at code there.

comment:2 follow-up: Changed 7 months ago by nickm

(We should add a BUG_ONCE for that second conditional in 0.3.0, I think)

comment:3 in reply to: ↑ 2 Changed 7 months ago by teor

  • Keywords CoreTorTeam201611 added
  • Status changed from needs_review to merge_ready

Replying to nickm:

(We should add a BUG_ONCE for that second conditional in 0.3.0, I think)

Yes, definitely.

Otherwise, let's get this merged, looks good.

I wonder how long this has been waiting to bite us?

comment:4 Changed 7 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

my 20534_029 branch is squashed and merged.

Added the IF_BUG_ONCE in 89edef6afbfc70

Note: See TracTickets for help on using tickets.