Skip to content

fix(pjsua): honour the retry interval when it is below the jitter #5319

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.

Sign up for GitHub

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

Merged
sauwming merged 2 commits into from
Oct 7, 2026
Merged

fix(pjsua): honour the retry interval when it is below the jitter #5319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9970b9
fix(pjsua): honour the retry interval when it is below the jitter
laconicman Oct 6, 2026
0e1ad28
fix(pjsua): guard the retry delay against huge intervals, fix comments
laconicman Oct 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions pjsip/include/pjsua-lib/pjsua.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5144,9 +5144,9 @@ typedef struct pjsua_acc_config
* 6xx (global failure), and failure caused by transport problem.
* For registration retry caused by transport failure, the first retry
* will be done after \a reg_first_retry_interval seconds instead.
* Note that the interval will be randomized slightly by some seconds
* (specified in \a reg_retry_random_interval) to avoid all clients
* re-registering at the same time.
* Note that the interval will be randomized by
* \a reg_retry_random_interval to avoid all clients re-registering at
* the same time; see there for the resulting range.
*
* See also \a reg_first_retry_interval setting.
*
Expand All @@ -5157,9 +5157,9 @@ typedef struct pjsua_acc_config
/**
* This specifies the interval for the first registration retry. The
* registration retry is explained in \a reg_retry_interval. Note that
* the value here will also be randomized by some seconds (specified
* in \a reg_retry_random_interval) to avoid all clients re-registering
* at the same time.
* the value here will also be randomized by
* \a reg_retry_random_interval to avoid all clients re-registering at
* the same time; see there for the resulting range.
*
* Default: 0
*/
Expand All @@ -5172,7 +5172,11 @@ typedef struct pjsua_acc_config
* This is useful to avoid all clients re-registering at the same time.
* For example, if the registration retry interval is set to 100 seconds
* and this is set to 10 seconds, the actual registration retry interval
* will be in the range of 90 to 110 seconds.
* will be in the range of 90 to 110 seconds. If the retry interval is
* shorter than this value, it cannot be reduced by that much, and the
* actual interval will be in the range of 0 to the retry interval plus
* this value instead: for example 0 to 15 seconds for a retry interval
* of 5 seconds.

@sauwming sauwming Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for documenting the new window here. The reg_retry_interval (L5147) and reg_first_retry_interval (L5161) docs still say the interval is "randomized slightly by some seconds", though, and with this change a 1 s interval can become anything from 0 to 11 s. Could you update those two to point to the clamped window described here?

@laconicman laconicman Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0e1ad28: both now say the interval is randomized by reg_retry_random_interval and point there for the resulting range, rather than repeating it. The pjsua2 retryIntervalSec and firstRetryIntervalSec docs got the same change, and pjsip/pjproject_docs#70 adds reg_retry_random_interval to the retry settings listed in the server affinity guide.

*
* Default: 10
*/
Expand Down
20 changes: 12 additions & 8 deletions pjsip/include/pjsua2/account.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ struct AccountRegConfig : public PersistentObject
* disable auto re-registration. Note that if the registration retry
* occurs because of transport failure, the first retry will be done
* after \a firstRetryIntervalSec seconds instead. Also note that
* the interval will be randomized slightly by some seconds (specified
* in \a reg_retry_random_interval) to avoid all clients re-registering
* at the same time.
* the interval will be randomized by \a randomRetryIntervalSec to
* avoid all clients re-registering at the same time; see there for
* the resulting range.
*
* See also \a firstRetryIntervalSec and \a randomRetryIntervalSec
* settings.
Expand All @@ -128,9 +128,9 @@ struct AccountRegConfig : public PersistentObject
/**
* This specifies the interval for the first registration retry. The
* registration retry is explained in \a retryIntervalSec. Note that
* the value here will also be randomized by some seconds (specified
* in \a reg_retry_random_interval) to avoid all clients re-registering
* at the same time.
* the value here will also be randomized by \a randomRetryIntervalSec
* to avoid all clients re-registering at the same time; see there for
* the resulting range.
*
* See also \a retryIntervalSec and \a randomRetryIntervalSec settings.
*
Expand All @@ -141,11 +141,15 @@ struct AccountRegConfig : public PersistentObject
/**
* This specifies maximum randomized value to be added/substracted
* to/from the registration retry interval specified in \a
* reg_retry_interval and \a reg_first_retry_interval, in second.
* retryIntervalSec and \a firstRetryIntervalSec, in second.
* This is useful to avoid all clients re-registering at the same time.
* For example, if the registration retry interval is set to 100 seconds
* and this is set to 10 seconds, the actual registration retry interval
* will be in the range of 90 to 110 seconds.
* will be in the range of 90 to 110 seconds. If the retry interval is
* shorter than \a randomRetryIntervalSec, it cannot be reduced by that
* much, and the actual interval will be in the range of 0 to the retry
* interval plus \a randomRetryIntervalSec instead: for example 0 to 15
* seconds for a retry interval of 5 seconds.
*
* See also \a retryIntervalSec and \a firstRetryIntervalSec settings.
*
Expand Down
19 changes: 14 additions & 5 deletions pjsip/src/pjsua-lib/pjsua_acc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5467,8 +5467,9 @@ static void auto_rereg_timer_cb(pj_timer_heap_t *th, pj_timer_entry *te)
}


/* Schedule reregistration for specified account. Note that the first
* re-registration after a registration failure will be done immediately.
/* Schedule reregistration for specified account. Until a registration
* succeeds again, the first retry waits reg_first_retry_interval and later
* ones reg_retry_interval, both randomized by reg_retry_random_interval.
* Also note that this function should be called within PJSUA mutex.
*/
static void schedule_reregistration(pjsua_acc *acc)
Expand Down Expand Up @@ -5516,19 +5517,27 @@ static void schedule_reregistration(pjsua_acc *acc)
acc->auto_rereg.timer.cb = &auto_rereg_timer_cb;
acc->auto_rereg.timer.user_data = acc;

/* Reregistration attempt. The first attempt will be done immediately. */
/* Reregistration attempt. The first one since the last successful
* registration waits reg_first_retry_interval.
*/
delay.sec = acc->auto_rereg.attempt_cnt? acc->cfg.reg_retry_interval :
acc->cfg.reg_first_retry_interval;
delay.msec = 0;

/* Randomize interval by +/- reg_retry_random_interval, if configured */
/* Randomize interval by +/- reg_retry_random_interval, if configured.
* The lower bound is clamped at 0: an interval shorter than that gives
* a delay between 0 and the interval plus reg_retry_random_interval.
*/
if (acc->cfg.reg_retry_random_interval) {
long rand_ms = acc->cfg.reg_retry_random_interval * 1000;
if (delay.sec >= (long)acc->cfg.reg_retry_random_interval) {

@sauwming sauwming Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nearby comments no longer match the behaviour (both lines are outside the diff, so I'm commenting here):

  • L5524 /* Randomize interval by +/- reg_retry_random_interval, if configured */: the else branch now uses 0 to interval + jitter, not +/- jitter. Please mention that the lower bound is clamped at 0.
  • L5519 The first attempt will be done immediately. (and the same statement in the function header at L5471): the first retry actually waits reg_first_retry_interval plus jitter. That predates this PR, but the new header docs now contradict it, so it'd be good to fix it here too.

@laconicman laconicman Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixed in 0e1ad28. The randomisation comment now says the lower bound is clamped at 0 and gives the resulting range for an interval below reg_retry_random_interval. The function header and the in-body comment now say that, until a registration succeeds again, the first retry waits reg_first_retry_interval and later ones reg_retry_interval, since attempt_cnt is only reset on a 2xx.

delay.msec = -rand_ms + (pj_rand() % (rand_ms * 2));
} else {
delay.sec = 0;
/* A huge unsigned interval may not fit in a long */
if (delay.sec < 0)
delay.sec = 0;
delay.msec = (pj_rand() % (delay.sec * 1000 + rand_ms));

@sauwming sauwming Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The divisor now depends on delay.sec, which is a signed long copied from the unsigned reg_retry_interval / reg_first_retry_interval. Where long is 32-bit (Windows, 32-bit Linux), a huge value, e.g. (unsigned)-10, makes delay.sec negative. It then takes this branch, delay.sec * 1000 + rand_ms can become 0, and the % crashes with SIGFPE. Values like 0x80000000 also overflow delay.sec * 1000. Before this change the divisor was always rand_ms, so this is new.

It's an odd config, but the guard is cheap. Could you clamp delay.sec to [0, reg_retry_random_interval) before using it here, e.g. by testing delay.sec < 0 too, or by doing the comparison in unsigned?

@laconicman laconicman Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that this is new: the divisor used to be rand_ms alone. Fixed in 0e1ad28 by clamping a negative delay.sec to 0 in that branch, so such a value gets the same 0 to reg_retry_random_interval delay as before this PR. I didn't do the comparison in unsigned: those values would then take the other branch and come out negative, and since the timer adds the delay to the current time, every retry would fire at once. I can't build with a 32-bit long here, so I checked the arithmetic with long emulated as 32-bit under UBSan: division by zero before the fix, the old 0–10 s after it. With the clamp the divisor stays below 2 * rand_ms, so it can only overflow for a reg_retry_random_interval at which the other branch's rand_ms * 2 already does (above about 12 days); I've left that pre-existing limit alone.

delay.sec = 0;
}
}
pj_time_val_normalize(&delay);
Expand Down
Loading