Skip to content

fix(dialog): cancel() in Calling defers the CANCEL to the first provisional - #203

Open
tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/cancel-in-calling-defers-cancel
Open

tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/cancel-in-calling-defers-cancel

Conversation

@tgeorge06

Copy link
Copy Markdown
Contributor

Fixes #197.

Thanks to @beija-wq for the report. The analysis in the issue is exact, and the reproduction program made it easy to check before and after; this PR follows the suggested fix (arm the deferred path in Calling, an error for a UAS).

Problem

InviteDialog::cancel() (src/dialog/invite_dialog.rs:204-216 at current main, 6fd9414) sends a CANCEL only for a UAC in Trying or Early, and returns Ok(()) otherwise:

  • UAC in Calling (INVITE sent, no response yet): nothing is sent and nothing is remembered. A do_invite caller can drop the future instead (DialogGuardForUnconfirmed, src/dialog/invitation.rs:186-339, fix: CANCEL a dropped INVITE after its first provisional response (rebase of #168) #171), but for do_invite_async an explicit cancel() is the way to abandon the call, so the cancelled call rings on and is established if the callee answers.
  • UAS: Ok(()) and nothing sent, so the caller cannot tell "CANCEL sent" from "nothing happened".

ClientInviteDialog::cancel delegates to the same function (src/dialog/client_dialog.rs:267).

Expected

RFC 3261 §9.1:

If no provisional response has been received, the CANCEL request MUST NOT be sent; rather, the client MUST wait for the arrival of a provisional response before sending the request.

So in Calling the request has to be deferred, not dropped. A 2xx that arrives before the CANCEL could go out, or that crosses it, is not affected by the CANCEL: the UAC ACKs it and ends the session with a BYE (§9.1, §15), which is what the drop guard already does since #162 and #171. CANCEL is a UAC request; a UAS ends an unanswered INVITE with a final response (reject()).

Fix

  • cancel() in Calling records the request on the dialog (cancel_requested, cancel_deferred) under the dialog state lock and returns Ok(()). process_invite sends the CANCEL when the first provisional (100 included) arrives, on a spawned task, through the same send_cancel() as Trying/Early. The state lock makes the hand-over race free: either process_invite leaves Calling after cancel() armed the CANCEL and sends it, or cancel() sees Trying/Early and sends it itself.
  • After cancel() (in Calling, Trying or Early), the 2xx is decided under the same lock (DialogInner::confirm_unless_cancelled): the dialog reports Terminated(UacCancel) instead of Confirmed, the transaction ACKs the 2xx as before, and the existing bye_if_2xx helper of the drop guard sends the BYE. do_invite / do_invite_async do not register the terminated dialog under its confirmed id, but still drain the Accepted transaction, so retransmitted 2xx are re-ACKed and a forked 2xx gets its BYE (fix(dialog): BYE a forked 2xx's dialog (RFC 3261 §13.2.2.4) #181).
  • A 401/407 to a cancelled INVITE is its final response: it is not resent with credentials.
  • For a reliable provisional (RFC 3262) the deferred CANCEL is dispatched before the PRACK, so it is not held back until the PRACK transaction ends; it is skipped when a final response was processed before the spawned task ran.
  • The legacy ClientInviteDialog::process_invite gets the same three hooks.
  • On a UAS dialog cancel() returns Err(Error::DialogError("cancel is a UAC operation; use reject()", ..)). hangup() on an unanswered UAS dialog keeps returning Ok(()) (it no longer goes through cancel()).

transition() keeps its behavior; its body moved into transition_locked() so confirm_unless_cancelled can decide and transition under one lock. +147 / -33 lines in src/dialog/; the rest of the +459 / -35 is tests.

Unchanged on purpose:

  • The drop guard (fix: CANCEL a dropped INVITE after its first provisional response (rebase of #168) #171, fix(dialog): end a dropped INVITE whose dialog was already removed #183) is untouched; it calls send_cancel() directly, so dropping a do_invite future after cancel() behaves as before.
  • In Calling the INVITE keeps retransmitting (Timer A) until the first response, since process_invite still drives the transaction and the caller still waits for its result. No response at all ends with Timer B's 408, and no CANCEL is ever sent.
  • A 487 (or any other non-2xx final) after the CANCEL is ACKed by the transaction and ends the dialog Terminated(UasOther(..)), exactly as for a cancel() in Early today.
  • After a final response cancel() is still Ok(()) without sending anything (a CANCEL would have no effect).
  • No transport-specific code: the CANCEL takes the same do_request path as today on every transport.

Contract / coverage

InviteDialog::cancel() / ClientInviteDialog::cancel():

dialog before after
UAC, Calling Ok(()), nothing sent or remembered Ok(()); CANCEL sent on the first provisional
UAC, Trying / Early CANCEL sent, its outcome same
UAC, after a final response Ok(()), nothing sent same
UAS, any state Ok(()), nothing sent Err(DialogError), nothing sent

What a cancelled INVITE ends with:

peer sends before after
100 or 180 (cancelled in Calling) nothing CANCEL
487 ACK, Terminated(UasOther(487)) same
486 before any provisional ACK same, no CANCEL
2xx before any provisional, or crossing the CANCEL ACK, Confirmed, registered in the layer ACK + BYE, Terminated(UacCancel), not registered
401/407 (with a credential) INVITE resent with credentials final response, no retry
no response 408 (Timer B) same, no CANCEL

hangup() / hangup_with_headers(): a UAC in Calling now gets the deferred CANCEL (before: nothing); everything else as before.

Known limits, same as an explicit cancel() in Early today: a final response processed at the same moment the deferred CANCEL is sent can still cross it on the wire (harmless, the UAS answers the CANCEL). With auto_ack_2xx = false (proxy mode, where the transaction does not ACK a 2xx) the ACK stays the caller's, as in the drop guard.

Tests

src/dialog/tests/test_cancel_2xx_race.rs, against a raw UDP peer, with do_invite_async:

  • test_cancel_in_calling_is_sent_after_the_180, ..._after_the_100: no CANCEL for over 2*T1, then CANCEL on the provisional; 487 ACKed, no BYE.
  • test_cancel_in_calling_2xx_crossing_the_cancel_is_acked_and_byed, test_cancel_in_calling_2xx_without_provisional_is_acked_and_byed: ACK + BYE in the 2xx's dialog, one Terminated(UacCancel), never Confirmed, nothing left in the layer.
  • test_cancel_in_early_2xx_crossing_the_cancel_is_acked_and_byed: the same for cancel() in Early (2xx and 487).
  • test_cancel_in_calling_final_failure_is_acked_only (486) and test_cancel_in_calling_without_response_times_out (Timer B 408, short T1): controls, already pass on main.
  • test_cancel_in_calling_is_not_held_back_by_the_prack: reliable 183, PRACK never answered; the PRACK and the CANCEL both arrive within 1 s.
  • test_cancel_in_calling_with_the_legacy_client_dialog: the same through ClientInviteDialog::process_invite.

src/dialog/tests/test_invite_auth_challenge.rs, test_cancelled_invite_is_not_retried_with_credential; src/dialog/tests/test_uas_ack_timeout.rs, test_cancel_on_a_server_dialog_fails (error, only 100 Trying on the wire, hangup() still Ok(()) and not terminated).

On main (6fd9414) 9 of the 11 new tests fail (the two controls pass); test and panic message:

test_cancel_in_calling_is_sent_after_the_180 ... timeout waiting for CANCEL
test_cancel_in_calling_2xx_without_provisional_is_acked_and_byed ... timeout waiting for BYE
test_cancel_in_early_2xx_crossing_the_cancel_is_acked_and_byed ... timeout waiting for BYE
test_cancelled_invite_is_not_retried_with_credential ... left: Some(OK)
test_cancel_on_a_server_dialog_fails ... cancel() on a UAS: ()

The reproduction program from the issue, against this branch:

A: cancel() before any response -> Ok(())
A: after the peer's 180: ["CANCEL sip:bob@127.0.0.1:52866 SIP/2.0", "CANCEL sip:bob@127.0.0.1:52866 SIP/2.0"]
B: UAS cancel() on an unanswered INVITE -> Err("Dialog error:500 Server Internal Error(cancel is a UAC operation; use reject())")
RESULT: not reproduced - UAC cancel() lost: false; UAS cancel() silent Ok: false

(The second CANCEL line is the CANCEL's own retransmission: the program does not answer it.)

Checks

  • cargo test --features bench: 431 lib tests passed, 0 failed (420 on main plus the 11 new ones), and 65 doc tests passed. Plain cargo test passes too.
  • cargo check --no-default-features --features platform-embassy: no warnings, as on main.
  • cargo clippy --features bench --all-targets: the same output as on main, none in the changed code. (On main it stops at a clippy::never_loop error in src/dialog/tests/test_refer_notify.rs:98, unrelated to this PR; with -A clippy::never_loop the warnings are the same as on main.)
  • cargo fmt --check: clean.
  • Merges cleanly with fix(transaction): answer a CANCEL after a final response with its own 200 #199 (no shared files).

…sional

InviteDialog::cancel() returned Ok(()) and did nothing for a UAC whose
INVITE had no response yet, and for any UAS dialog. For do_invite_async it
is the only way to abandon a call, so the cancelled call could still be
established.

In Calling, cancel() now records the request and returns Ok(()).
process_invite sends the CANCEL once the first provisional arrives (RFC
3261 section 9.1). After cancel(), a 2xx to the INVITE is ended with a BYE
(the transaction ACKs it), the dialog reports Terminated(UacCancel) and is
not registered as confirmed, the same outcome as a dropped do_invite
future. A challenge to a cancelled INVITE is not answered with credentials.
The legacy ClientInviteDialog::process_invite does the same.

On a UAS dialog cancel() returns an error: CANCEL is a UAC request and an
unanswered INVITE ends with reject(). hangup() on such a dialog keeps
returning Ok(()).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InviteDialog::cancel() returns Ok(()) and does nothing: UAC before the first provisional, and any UAS dialog

1 participant