Repository navigation
fix(pjsua): honour --rereg-delay for accounts given on the command line - #5306
Conversation
sauwming
left a comment
There was a problem hiding this comment.
Note on the expected behaviour in the description ("--rereg-delay=1 gives retries within 0–10 s"): that comes from an existing bug in schedule_reregistration(), not from the intended randomisation:
pjproject/pjsip/src/pjsua-lib/pjsua_acc.c
Lines 5530 to 5531 in a67b8e8
delay.sec is zeroed before it is used in the modulo, so any interval below reg_retry_random_interval gives 0–10 s regardless of the value (--rereg-delay=9 behaves the same as =1). This is a library issue and can be fixed separately, but this PR makes it reachable from pjsua.
| /* Add accounts */ | ||
| for (i=0; i<app_config.acc_cnt; ++i) { | ||
| app_config.acc_cfg[i].rtp_cfg = app_config.rtp_cfg; | ||
| app_config.acc_cfg[i].reg_retry_interval = 300; |
There was a problem hiding this comment.
With this override gone, --rereg-delay=0 (or a non-numeric value, which pj_strtoul() parses as 0) now sets reg_retry_interval to 0, which disables auto re-registration entirely: after a 503/408 or a transport disconnect the account stays unregistered, with no error at startup. Previously this was masked by the hardcoded 300.
Could we either validate the value in OPT_REG_RETRY_INTERVAL (reject 0 / non-numeric), or document in the --rereg-delay help text that 0 disables re-registration?
There was a problem hiding this comment.
Good catch. A 0 now reaches reg_retry_interval, where pjsua.h documents it as disabling auto re-registration, and the option could not produce 0 before because the app overwrote it. I've added a line to the --rereg-delay usage text saying so (2dcd7ca). I left non-numeric values alone: pj_strtoul() returns 0 for them and --reg-use-proxy is parsed the same way. Happy to reject them if you'd prefer.
| for (i=0; i<app_config.acc_cnt; ++i) { | ||
| app_config.acc_cfg[i].rtp_cfg = app_config.rtp_cfg; | ||
| app_config.acc_cfg[i].reg_retry_interval = 300; | ||
| app_config.acc_cfg[i].reg_first_retry_interval = 60; |
There was a problem hiding this comment.
Removing this changes the first retry after a failure from ~60 s (50–70 s with randomisation) to the library default of 0–10 s for every command-line account, which isn't needed to honour --rereg-delay. Since there's no option to set reg_first_retry_interval, users relying on the old behaviour can't get it back.
Suggest keeping this line and only dropping the reg_retry_interval override, unless the change in first-retry behaviour is intended (in which case it'd be worth mentioning in the description).
There was a problem hiding this comment.
Agreed, I've dropped that commit and left reg_first_retry_interval = 60 as it is: nothing sets it, and it isn't needed to honour the option. The description now says that --rereg-delay governs the retries after the first one, and that the first still comes after about 60 s.
The "Add accounts" loop in app_init() set reg_retry_interval = 300 on every command-line account. The loop runs after option parsing, so the value parsed for --rereg-delay was discarded, and write_account_settings() never wrote the option back because the value always equalled the default. The assignment came in with r3804 on the 1.x branch, a change to the transport adapter sample (pjsip#1383) that does not mention it. The option is older (pjsip#1044). Accounts added at run time by ui_add_account() and cmd_add_account() were never overridden. reg_first_retry_interval = 60, set beside it, is left alone: no option sets it, so the first retry after a failure still comes after about 60 s, and --rereg-delay governs the retries after that.
With --rereg-delay honoured, a 0 now reaches reg_retry_interval, where pjsua-lib documents it as disabling auto re-registration. The option could not produce 0 before, because the app overwrote the value. Say so in the usage text.
53fb23d to
2dcd7ca
Compare
|
You're right that the description presented 0–10 s as expected behaviour. I've corrected it: it now says that |
Fixes #5305.
--rereg-delayhas no effect on accounts given on the command line.pjsua_app_config.cparses it into the account'sreg_retry_interval, but the "Add accounts" loop inapp_init()then setsreg_retry_interval = 300on every such account, after option parsing. For the same reasonwrite_account_settings()never writes the option back. Accounts added at run time (+a,acc add) start frompjsua_acc_config_default()and were never overridden.I found this while writing a SIPp scenario for #5313. pjsua started before SIPp was listening on TCP, so the first REGISTER was refused and the retry came about a minute later, whatever
--rereg-delaysaid. The option is listed in--helpand written back bywrite_account_settings(), so a user has every reason to expect it to work.The assignment arrived in 1.x r3804 (#1383, the update of the transport adapter sample), which doesn't mention it.
--rereg-delayitself is older (#1044).Two commits:
67453e8a— dropreg_retry_interval = 300. The parsed value now reachespjsua_acc_add(), and the config dump writes it back. Thereg_first_retry_interval = 60next to it is left alone on purpose: no option sets it, so the first retry after a failure still comes after about 60 s, and--rereg-delaygoverns the retries after that. In my SIPp case that means the first retry is still a minute away, so this does not shorten it; I worked around it by registering at run time with+a.2dcd7caa— say in--helpthat--rereg-delay=0disables retries. That is what 0 means forreg_retry_intervalin pjsua-lib, and the option could not produce 0 before. A non-numeric value parses as 0 and so disables retries too, as it would for--reg-use-proxy; I did not add validation.Testing
pjsua on macOS against a closed TCP port, reading the "Scheduling re-registration retry for acc 1 in N seconds" log lines:
--rereg-delaya67b8e81bThe first retry is the app's 60 s (randomised by
reg_retry_random_interval, 10 s), and the ones after it follow--rereg-delay. For an interval belowreg_retry_random_interval,schedule_reregistration()currently draws the delay from 0–10 s, so--rereg-delay=1and=9behave alike. That is how the library schedules short intervals today and this PR does not touch it.With
--rereg-delay=30thedcconfig dump now includes--rereg-delay 30; on master it has none.makebuilds without new warnings, andtests/pjsuascripts-run/100_simple.pypasses.