Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5432 closed Bug (fixed)

getRetryInterval bug

Reported by: pschrst Owned by: jordan
Priority: Normal Milestone: 2.82
Component: Transmission Version: 2.81
Severity: Normal Keywords:
Cc:

Description

Thank you Jordan for applying a fix for issue #5395. However, there is a bug in your patch, first condition in the switch will never fire, since consecutiveFailures is incremented earlier than getRetryInterval is called. This way, the "quick reconnect on error" feature is simple bypassed.

Change History (7)

comment:1 follow-up: Changed 8 years ago by jordan

  • Milestone changed from None Set to 2.82
  • Owner set to jordan
  • Status changed from new to assigned

This is a great point, I can't believe nobody's ever noticed this before. :/

comment:2 Changed 8 years ago by jordan

Fixed in r14137 and r14138.

Please reopen this ticket if the problem persists.

comment:3 Changed 8 years ago by jordan

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

comment:4 in reply to: ↑ 1 Changed 8 years ago by rb07

Replying to jordan:

I can't believe nobody's ever noticed this before. :/

Looking at the log of a 2.80 daemon, it is working correctly, first tries each 20 s, 4 or 5 times, then tries each 300 + random seconds.

I haven't used the changes, I don't know if it still works, or not.

comment:5 Changed 8 years ago by cfpp2p

I get the same as rb07

And taking a look at:

static void
on_announce_error (tr_tier * tier, const char * err, tr_announce_event e)
{
    int interval;

    /* increment the error count */
    if (tier->currentTracker != NULL)
        ++tier->currentTracker->consecutiveFailures;

    /* set the error message */
    dbgmsg (tier, "%s", err);
    tr_logAddTorInfo (tier->tor, "%s", err);
    tr_strlcpy (tier->lastAnnounceStr, err, sizeof (tier->lastAnnounceStr));

    /* switch to the next tracker */
    tierIncrementTracker (tier);

    /* schedule a reannounce */
    interval = getRetryInterval (tier->currentTracker);

it looks likes the incrementing for consecutiveFailures

++tier->currentTracker->consecutiveFailures;

is first but then we change to a different tracker

/* switch to the next tracker */
    tierIncrementTracker (tier);

so the incrementing of consecutiveFailures above is for the previous tracker.

comment:6 Changed 8 years ago by pschrst

But there are sites with only 1 tracker. In this situation the first retry attempt was always case 1 (tr_cryptoWeakRandInt (60) + (60 * 5)) not case 0 (return 20; ).

comment:7 Changed 8 years ago by x190

---

Last edited 8 years ago by x190 (previous) (diff)
Note: See TracTickets for help on using tickets.