Repository navigation
Conversation
nanangizz
left a comment
There was a problem hiding this comment.
Thanks for the PR, the option looks useful for automation. I built it and tested it against a second pjsua as the callee (--auto-answer=<code>):
| Callee answer | Exit code |
|---|---|
200, caller --duration=2 |
0 |
| 486 | 2 |
| 480 | 1 (expected 3) |
| 603 | 1 |
| no URI | 1 |
pjsua --help / --version (without the option) |
1 (was 0) |
A few things to fix:
- Any call's disconnect ends the app, not just the outgoing call.
on_call_state()doesn't check the call ID. Reproduced: while its outgoing call was still ringing, pjsua with--exit-on-call-end --auto-answer=486rejected an unrelated incoming call and exited with code 2 ("busy"), cancelling its own call with 487. Please store the call ID returned bypjsua_call_make_call()and only react to that call. - Exit status changes without the option.
main.cnow returns 1 wheneverpjsua_app_init()fails, and--helpand--versionwork by returningPJ_EINVALfrom argument parsing, so both now exit 1 instead of 0. Please keep returning 0 when--exit-on-call-endisn't set, so existing behaviour is unchanged as the description says. - 480 (Temporarily Unavailable) maps to 1. It's the usual "unavailable" response, so it should map to 3 along with 408 and 503. 600 (Busy Everywhere) would also fit "busy".
- The missing-URI check runs in
pjsua_app_run(), after transports and accounts are created and registration may have started. Checking it at the end of argument parsing would fail fast instead. - The issue asks for the behaviour to be tested. A
tests/pjsuaSIPp scenario (e.g. a UAS answering 486) that checks the exit code would cover it.
Minor: the OPT_EXIT_ON_CALL_END enum line is 86 columns, and the change drops the trailing blank line at the end of pjsua_app_common.h, which is unrelated.
|
Thanks for the quick and thorough review. Pushed f7c6831 addressing the feedback:
I haven't added an automated test. The
Test script (run from the repo root)#!/bin/zsh
# Manual test for pjsua --exit-on-call-end. Run from the pjproject root.
#
# Two pjsua instances talk over loopback:
# callee: answers every incoming call with a fixed SIP status (--auto-answer)
# caller: places one call with --exit-on-call-end; we check its exit code
#
# Exit codes: 0 = call succeeded, 1 = failed, 2 = busy, 3 = unavailable
P=./pjsip-apps/bin/pjsua-$(make infotarget)
CALLEE_PORT=5090
CALLER_PORT=5091
OTHER_PORT=5092
FAILS=0
to() { perl -e 'alarm shift; exec @ARGV' "$@"; } # macOS has no `timeout`
cleanup() { pkill -f "local-port=509" 2>/dev/null; sleep 1; }
check() { # description, expected exit code, actual exit code
if [ "$2" = "$3" ]; then
printf "PASS %-52s exit %s\n" "$1" "$3"
else
printf "FAIL %-52s expected %s, got %s\n" "$1" "$2" "$3"
FAILS=$((FAILS+1))
fi
}
# call_test <description> <expected exit> <callee answer> [caller options...]
call_test() {
desc=$1; expect=$2; answer=$3; shift 3
(sleep 20; echo q) | $P --null-audio --no-tcp --local-port=$CALLEE_PORT \
--auto-answer=$answer --log-level=1 >/dev/null 2>&1 &
sleep 2
to 15 $P --null-audio --no-tcp --local-port=$CALLER_PORT --log-level=1 \
"$@" sip:x@127.0.0.1:$CALLEE_PORT >/dev/null 2>&1 </dev/null
check "$desc" $expect $?
cleanup
}
cleanup
echo "Binary: $P"
echo
echo "== Exit code follows how the outgoing call ended =="
call_test "callee answers 200, caller hangs up after 2s" 0 200 --exit-on-call-end --duration=2
call_test "callee replies 486 Busy Here" 2 486 --exit-on-call-end
call_test "callee replies 600 Busy Everywhere" 2 600 --exit-on-call-end
call_test "callee replies 480 Temporarily Unavailable" 3 480 --exit-on-call-end
call_test "callee replies 603 Decline" 1 603 --exit-on-call-end
echo
echo "== Invalid use and unchanged behavior =="
to 10 $P --null-audio --exit-on-call-end --log-level=1 </dev/null >/dev/null 2>&1
check "option given without a URI to call" 1 $?
$P --help >/dev/null 2>&1; check "--help without the option still exits 0" 0 $?
$P --version >/dev/null 2>&1; check "--version without the option still exits 0" 0 $?
echo
echo "== Call that never gets a response (takes ~35s) =="
to 50 $P --null-audio --no-tcp --local-port=$CALLER_PORT --exit-on-call-end \
--log-level=1 sip:x@127.0.0.1:5999 >/dev/null 2>&1 </dev/null
check "nothing listening at the destination (timeout)" 3 $?
echo
echo "== Only the outgoing call counts =="
# B rings forever. A calls B and rejects any incoming call with 486.
# C then calls A. A must reject C but keep waiting for B.
(sleep 25; echo q) | $P --null-audio --no-tcp --local-port=$CALLEE_PORT \
--auto-answer=180 --log-level=1 >/dev/null 2>&1 &
sleep 2
to 12 $P --null-audio --no-tcp --local-port=$CALLER_PORT --auto-answer=486 \
--exit-on-call-end --log-level=1 sip:x@127.0.0.1:$CALLEE_PORT \
>/dev/null 2>&1 </dev/null &
sleep 3
(sleep 2; echo q) | $P --null-audio --no-tcp --local-port=$OTHER_PORT \
--log-level=1 sip:x@127.0.0.1:$CALLER_PORT >/dev/null 2>&1
sleep 2
if pgrep -f "local-port=$CALLER_PORT" >/dev/null; then
printf "PASS %-52s still running\n" "unrelated incoming call rejected, app keeps waiting"
else
printf "FAIL %-52s app exited\n" "unrelated incoming call rejected, app keeps waiting"
FAILS=$((FAILS+1))
fi
cleanup
echo
[ $FAILS -eq 0 ] && echo "ALL PASSED" || echo "$FAILS FAILED"
exit $FAILSOutput: |
nanangizz
left a comment
There was a problem hiding this comment.
Thanks for the update, this addresses everything from the first round. I rebuilt the branch and re-ran the scenarios (200/486/600/480/603, no answer, missing URI, --help/--version, unrelated incoming call, and the option loaded from --config-file with the URI on the command line) and they all return the expected codes.
One small question: is the new pjsua_app_exit_code global needed? main.c already reads app_config.exit_on_call_end through the existing extern, so I think it could read app_config.exit_code at the same spot, right after pjsua_app_run() returns and before pjsua_app_destroy(). That would drop the extern, the definition, and the copy at the end of pjsua_app_run(), and leave the volatile field as the single place the code is stored. Or is there a case I'm missing where the copy matters?
sauwming
left a comment
There was a problem hiding this comment.
Thanks for the PR. A few issues below; the first three (exit code 0 on startup failures, and a possible use-after-free at shutdown) matter most for the scripted use this option targets.
| running = PJ_FALSE; | ||
| } else { | ||
| running = PJ_FALSE; | ||
| exit_code = app_config.exit_on_call_end ? |
There was a problem hiding this comment.
If pjsua_app_init() fails after argument parsing (e.g. --local-port already in use), its on_error path calls app_destroy(), which does pj_bzero(&app_config, ...). So exit_on_call_end reads as FALSE here and the process exits 0 although no call was made. Same if an invalid option appears before --exit-on-call-end and parsing stops before the flag is set. The flag needs to be captured somewhere that survives app_destroy().
| pj_thread_destroy(stdout_refresh_thread); | ||
| stdout_refresh_quit = PJ_FALSE; | ||
| } | ||
| pjsua_app_exit_code = app_config.exit_code; |
There was a problem hiding this comment.
Early exits from pjsua_app_run() also report success: if pjsua_start() fails, the goto on_return skips the call and app_config.exit_code still holds its default 0, which is copied here. The PJ_ASSERT_RETURN when the stdout refresh thread can't be created returns before this line entirely. Suggest defaulting exit_code to PJSUA_APP_EXIT_CALL_FAILED in this mode and only setting success when the call actually completes.
| app_config.exit_code = PJSUA_APP_EXIT_UNAVAILABLE; | ||
| else | ||
| app_config.exit_code = PJSUA_APP_EXIT_CALL_FAILED; | ||
| app_config.call_finished = PJ_TRUE; |
There was a problem hiding this comment.
call_finished is set at the top of the DISCONNECTED branch, while this callback (on the worker thread) still goes on to ring_stop(), pjsua_player_set_pos(app_config.wav_id) and the player/recorder cleanup. The main thread can exit its loop and run app_destroy() concurrently, destroying the ring/ringback ports and players under it. Please set the flag at the end of the DISCONNECTED block.
| if (call_info.state == PJSIP_INV_STATE_DISCONNECTED) { | ||
|
|
||
| if (app_config.exit_on_call_end && | ||
| pjsua_call_get_user_data(call_id) == &app_config.exit_on_call_end) |
There was a problem hiding this comment.
Using call user_data as the marker breaks with transfers: pjsua copies user_data to the new call on REFER (pjsua_call.c:6797) and on incoming INVITE with Replaces. After a transfer, the original call is hung up (410), hits DISCONNECTED with the marker, and pjsua exits while the transferred call is still active. Storing the call ID from pjsua_call_make_call()'s p_call_id and comparing call_id here would be exact.
| else if (call_info.last_status == 486 || | ||
| call_info.last_status == 600) | ||
| app_config.exit_code = PJSUA_APP_EXIT_BUSY; | ||
| else if (call_info.last_status == 408 || |
There was a problem hiding this comment.
Exit code is derived only from last_status, so a call that was answered and later torn down by a failure (session timer expiry, re-INVITE/UPDATE timeout → 408, 503, 481) is reported as unavailable/failed even though it connected. Consider checking whether the call ever reached CONFIRMED (e.g. connect_duration > 0) first.
|
|
||
| if (app_config.use_cli) | ||
| if (app_config.exit_on_call_end) { | ||
| while (!app_config.call_finished) |
There was a problem hiding this comment.
With no UI in this mode, existing options that wait for user input can hang forever, e.g. --accept-redirect=3 returns PJSIP_REDIRECT_PENDING and waits for Ra/Rr. Also, an answered call without --duration only ends if the remote hangs up, and killing pjsua gives no defined exit code. Maybe reject incompatible options, or document that --duration is recommended.
| cfg_add(&cfg, max, "--set-qos\n"); | ||
| } | ||
| if (config->exit_on_call_end) { | ||
| cfg_add(&cfg, max, "--exit-on-call-end\n"); |
There was a problem hiding this comment.
--exit-on-call-end is written to the config file but the call URI isn't, so loading a saved config without a URI on the command line fails with "--exit-on-call-end requires an outgoing call URI". Since this mode never runs the CLI/legacy UI where settings are saved, it's probably better not to persist this flag.
| static pjsua_app_cfg_t app_cfg; | ||
| pj_str_t uri_arg; | ||
| pj_bool_t app_running = PJ_FALSE; | ||
| int pjsua_app_exit_code = PJSUA_APP_EXIT_SUCCESS; |
There was a problem hiding this comment.
This global duplicates app_config.exit_code, and is only updated at on_return (see the early-return issue below). main.c already reads app_config directly, so it could read app_config.exit_code before pjsua_app_destroy() instead, removing the extern, the global and the copy.
|
|
||
| status = pjsua_call_make_call(current_acc, &uri_arg, &call_opt, NULL, | ||
| status = pjsua_call_make_call(current_acc, &uri_arg, &call_opt, | ||
| app_config.exit_on_call_end ? |
There was a problem hiding this comment.
Minor: using &app_config.exit_on_call_end as an opaque marker is a bit obscure. Saving the call ID via the p_call_id argument (initialised to PJSUA_INVALID_ID) would be clearer, and also fixes the transfer issue above.
| if (app_config.use_cli) | ||
| if (app_config.exit_on_call_end) { | ||
| while (!app_config.call_finished) | ||
| pjsua_handle_events(100); |
There was a problem hiding this comment.
Minor: with the default worker thread (thread_cnt=1) the main thread duplicates event polling here and wakes every 100 ms, adding up to 100 ms exit latency. A semaphore posted from on_call_state, or pj_thread_sleep() when thread_cnt > 0, would avoid that.
Closes #5321
Add
--exit-on-call-endto pjsua for automated outgoing-call workflows.When enabled:
0: call succeeded1: call failed2: destination busy3: destination unavailableThe change is limited to the pjsua application layer and does not alter the PJSIP library APIs or existing VoIP behavior.