Repository navigation
Conversation
…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(()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-216at currentmain, 6fd9414) sends a CANCEL only for a UAC inTryingorEarly, and returnsOk(())otherwise:Calling(INVITE sent, no response yet): nothing is sent and nothing is remembered. Ado_invitecaller 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 fordo_invite_asyncan explicitcancel()is the way to abandon the call, so the cancelled call rings on and is established if the callee answers.Ok(())and nothing sent, so the caller cannot tell "CANCEL sent" from "nothing happened".ClientInviteDialog::canceldelegates to the same function (src/dialog/client_dialog.rs:267).Expected
RFC 3261 §9.1:
So in
Callingthe 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()inCallingrecords the request on the dialog (cancel_requested,cancel_deferred) under the dialog state lock and returnsOk(()).process_invitesends the CANCEL when the first provisional (100 included) arrives, on a spawned task, through the samesend_cancel()asTrying/Early. The state lock makes the hand-over race free: eitherprocess_inviteleavesCallingaftercancel()armed the CANCEL and sends it, orcancel()seesTrying/Earlyand sends it itself.cancel()(inCalling,TryingorEarly), the 2xx is decided under the same lock (DialogInner::confirm_unless_cancelled): the dialog reportsTerminated(UacCancel)instead ofConfirmed, the transaction ACKs the 2xx as before, and the existingbye_if_2xxhelper of the drop guard sends the BYE.do_invite/do_invite_asyncdo 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).ClientInviteDialog::process_invitegets the same three hooks.cancel()returnsErr(Error::DialogError("cancel is a UAC operation; use reject()", ..)).hangup()on an unanswered UAS dialog keeps returningOk(())(it no longer goes throughcancel()).transition()keeps its behavior; its body moved intotransition_locked()soconfirm_unless_cancelledcan decide and transition under one lock. +147 / -33 lines insrc/dialog/; the rest of the +459 / -35 is tests.Unchanged on purpose:
send_cancel()directly, so dropping ado_invitefuture aftercancel()behaves as before.Callingthe INVITE keeps retransmitting (Timer A) until the first response, sinceprocess_invitestill 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.Terminated(UasOther(..)), exactly as for acancel()inEarlytoday.cancel()is stillOk(())without sending anything (a CANCEL would have no effect).do_requestpath as today on every transport.Contract / coverage
InviteDialog::cancel()/ClientInviteDialog::cancel():CallingOk(()), nothing sent or rememberedOk(()); CANCEL sent on the first provisionalTrying/EarlyOk(()), nothing sentOk(()), nothing sentErr(DialogError), nothing sentWhat a cancelled INVITE ends with:
Calling)Terminated(UasOther(487))Confirmed, registered in the layerTerminated(UacCancel), not registeredhangup()/hangup_with_headers(): a UAC inCallingnow gets the deferred CANCEL (before: nothing); everything else as before.Known limits, same as an explicit
cancel()inEarlytoday: 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). Withauto_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, withdo_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, oneTerminated(UacCancel), neverConfirmed, nothing left in the layer.test_cancel_in_early_2xx_crossing_the_cancel_is_acked_and_byed: the same forcancel()inEarly(2xx and 487).test_cancel_in_calling_final_failure_is_acked_only(486) andtest_cancel_in_calling_without_response_times_out(Timer B 408, short T1): controls, already pass onmain.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 throughClientInviteDialog::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()stillOk(())and not terminated).On
main(6fd9414) 9 of the 11 new tests fail (the two controls pass); test and panic message:The reproduction program from the issue, against this branch:
(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 onmainplus the 11 new ones), and 65 doc tests passed. Plaincargo testpasses too.cargo check --no-default-features --features platform-embassy: no warnings, as onmain.cargo clippy --features bench --all-targets: the same output as onmain, none in the changed code. (Onmainit stops at aclippy::never_looperror insrc/dialog/tests/test_refer_notify.rs:98, unrelated to this PR; with-A clippy::never_loopthe warnings are the same as onmain.)cargo fmt --check: clean.