Opened 4 years ago

Closed 4 years 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:


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:


  /* 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)

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 4 years ago by nickm

Status: newneeds_review

bug20534_029 is my attempt at code there.

comment:2 Changed 4 years 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 4 years ago by teor

Keywords: CoreTorTeam201611 added
Status: needs_reviewmerge_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 4 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

my 20534_029 branch is squashed and merged.

Added the IF_BUG_ONCE in 89edef6afbfc70

Note: See TracTickets for help on using tickets.