From 5583f60776d348a4cd603b133e24a7a118164a16 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Mon, 13 Feb 2017 11:43:57 +0000 Subject: [PATCH 01/17] Add on_rx_trying callback --- include/scscfsproutlet.h | 5 +++++ include/sproutlet.h | 15 +++++++++++--- src/scscfsproutlet.cpp | 45 +++++++++++++++++++++++----------------- src/sproutletproxy.cpp | 17 +++++++++++---- 4 files changed, 56 insertions(+), 26 deletions(-) diff --git a/include/scscfsproutlet.h b/include/scscfsproutlet.h index 4d4d361f7..8d1926195 100644 --- a/include/scscfsproutlet.h +++ b/include/scscfsproutlet.h @@ -247,6 +247,7 @@ class SCSCFSproutletTsx : public SproutletTsx virtual void on_rx_in_dialog_request(pjsip_msg* req) override; virtual void on_tx_request(pjsip_msg* req, int fork_id) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; + virtual void on_rx_trying(pjsip_msg* rsp) override; virtual void on_tx_response(pjsip_msg* rsp) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; virtual void on_timer_expiry(void* context) override; @@ -365,6 +366,10 @@ class SCSCFSproutletTsx : public SproutletTsx /// @param sip_code - The reported SIP return code std::string fork_failure_reason_as_string(int fork_id, int sip_code); + /// Do common processing that we do for all responses. This is called by + /// on_rx_response and on_rx_trying. + void common_response_processing(pjsip_msg* rsp); + /// Pointer to the parent SCSCFSproutlet object - used for various operations /// that require access to global configuration or services. SCSCFSproutlet* _scscf; diff --git a/include/sproutlet.h b/include/sproutlet.h index 1cc750eb5..f76d0b2a7 100644 --- a/include/sproutlet.h +++ b/include/sproutlet.h @@ -330,15 +330,24 @@ class SproutletTsx /// request was sent. virtual void on_tx_request(pjsip_msg* req, int fork_id) { } - /// Called with all responses received on the transaction. If a transport - /// error or transaction timeout occurs on a downstream leg, this method is - /// called with a 408 response. + /// Called with all responses, except 100 Trying, received on the transaction. + /// If a transport error or transaction timeout occurs on a downstream leg, + /// this method is called with a 408 response. + /// + /// Note: 100 Trying responses are handled by the on_rx_trying method. /// /// @param rsp - The received request. /// @param fork_id - The identity of the downstream fork on which /// the response was received. virtual void on_rx_response(pjsip_msg* rsp, int fork_id) { send_response(rsp); } + /// Called if a 100 Trying response is received on the transaction. If the + /// These responses are only sent by the wrappers, so we shouldn't call + /// ever call send_response on them. + /// + /// @param rsp - The received request. + virtual void on_rx_trying(pjsip_msg* rsp) {} + /// Called when a response has been transmitted on the transaction. /// /// @param rsp - The transmitted response. diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index b29ff56b4..4670a1c65 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -594,20 +594,7 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) _se_helper.process_response(rsp, get_pool(rsp), trail()); } - // Pass the received response to the ACR. - // @TODO - timestamp from response??? - ACR* acr = get_acr(); - if (acr != NULL) - { - acr->rx_response(rsp); - } - - if (_liveness_timer != 0) - { - // The liveness timer is running on this request, so cancel it. - cancel_timer(_liveness_timer); - _liveness_timer = 0; - } + common_response_processing(rsp); int st_code = rsp->line.status.code; @@ -692,7 +679,7 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) // response we've seen from an AS track it as a successful // communication. This means that no matter how many 1xx responses we // receive we only track one success. - if ((st_code > PJSIP_SC_TRYING) && (!_seen_1xx)) + if (!_seen_1xx) { _scscf->track_app_serv_comm_success(_as_chain_link.uri(), _as_chain_link.default_handling()); @@ -701,10 +688,7 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } } - if (st_code > PJSIP_SC_TRYING) - { - _seen_1xx = true; - } + _seen_1xx = true; if (rsp != NULL) { @@ -715,6 +699,12 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } +void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp) +{ + common_response_processing(rsp); +} + + void SCSCFSproutletTsx::on_tx_response(pjsip_msg* rsp) { ACR* acr = get_acr(); @@ -2159,3 +2149,20 @@ std::string SCSCFSproutletTsx::fork_failure_reason_as_string(int fork_id, int si return reason; } + +void SCSCFSproutletTsx::common_response_processing(pjsip_msg* rsp) +{ + // Pass the received response to the ACR. + ACR* acr = get_acr(); + if (acr != NULL) + { + acr->rx_response(rsp); + } + + if (_liveness_timer != 0) + { + // The liveness timer is running on this request, so cancel it. + cancel_timer(_liveness_timer); + _liveness_timer = 0; + } +} diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index b704bf438..8a8922016 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1644,7 +1644,8 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) } register_tdata(rsp); - if ((PJSIP_IS_STATUS_IN_CLASS(rsp->msg->line.status.code, 100)) && + int status_code = rsp->msg->line.status.code; + if ((PJSIP_IS_STATUS_IN_CLASS(status_code, 100)) && (_forks[fork_id].state.tsx_state == PJSIP_TSX_STATE_CALLING)) { // Provisional response on fork still in calling state, so move to @@ -1654,7 +1655,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) _id.c_str(), pjsip_tx_data_get_info(rsp), fork_id, pjsip_tsx_state_str(_forks[fork_id].state.tsx_state)); } - else if (rsp->msg->line.status.code >= PJSIP_SC_OK) + else if (status_code >= PJSIP_SC_OK) { // Final response, so mark the fork as completed and decrement the number // of pending responses. @@ -1670,7 +1671,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) (_sproutlet->_outgoing_sip_transactions_tbl != NULL)) { // Update SNMP SIP transactions statistics for the Sproutlet. - if (rsp->msg->line.status.code >= 200 && rsp->msg->line.status.code < 300) + if (status_code >= 200 && status_code < 300) { _sproutlet->_outgoing_sip_transactions_tbl->increment_successes(_req_type); } @@ -1680,7 +1681,15 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) } } } - _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); + + if (status_code == PJSIP_SC_TRYING) + { + _sproutlet_tsx->on_rx_trying(rsp->msg); + } + else + { + _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); + } process_actions(false); } From 08d237bfd410d301ac173de327c0d179103f32cd Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Mon, 13 Feb 2017 13:21:40 +0000 Subject: [PATCH 02/17] Add observation API --- include/bgcfsproutlet.h | 4 ++-- include/icscfsproutlet.h | 8 +++---- include/scscfsproutlet.h | 6 ++--- include/sproutlet.h | 47 +++++++++++++++++++++++++--------------- src/bgcfsproutlet.cpp | 4 ++-- src/icscfsproutlet.cpp | 8 +++---- src/scscfsproutlet.cpp | 6 ++--- src/sproutletproxy.cpp | 19 +++++++++++----- src/ut/mock_sproutlet.h | 4 ++-- 9 files changed, 64 insertions(+), 42 deletions(-) diff --git a/include/bgcfsproutlet.h b/include/bgcfsproutlet.h index 29d9e4dd9..937585ac5 100644 --- a/include/bgcfsproutlet.h +++ b/include/bgcfsproutlet.h @@ -134,9 +134,9 @@ class BGCFSproutletTsx : public SproutletTsx ~BGCFSproutletTsx(); virtual void on_rx_initial_request(pjsip_msg* req) override; - virtual void on_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void on_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: diff --git a/include/icscfsproutlet.h b/include/icscfsproutlet.h index 0787d65f6..5bace6460 100644 --- a/include/icscfsproutlet.h +++ b/include/icscfsproutlet.h @@ -154,9 +154,9 @@ class ICSCFSproutletTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void on_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void on_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: @@ -200,9 +200,9 @@ class ICSCFSproutletRegTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void on_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void on_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: diff --git a/include/scscfsproutlet.h b/include/scscfsproutlet.h index 8d1926195..46ba6b901 100644 --- a/include/scscfsproutlet.h +++ b/include/scscfsproutlet.h @@ -245,10 +245,10 @@ class SCSCFSproutletTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void on_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void on_rx_trying(pjsip_msg* rsp) override; - virtual void on_tx_response(pjsip_msg* rsp) override; + virtual void on_rx_trying(pjsip_msg* rsp, int fork_id) override; + virtual void obs_tx_response(pjsip_msg* rsp) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; virtual void on_timer_expiry(void* context) override; diff --git a/include/sproutlet.h b/include/sproutlet.h index f76d0b2a7..31d8916d8 100644 --- a/include/sproutlet.h +++ b/include/sproutlet.h @@ -321,22 +321,13 @@ class SproutletTsx /// @param req - The received in-dialog request. virtual void on_rx_in_dialog_request(pjsip_msg* req) { send_request(req); } - /// Called when a request has been transmitted on the transaction (usually - /// because the service has previously called forward_request() with the - /// request message. - /// - /// @param req - The transmitted request - /// @param fork_id - The identity of the downstream fork on which the - /// request was sent. - virtual void on_tx_request(pjsip_msg* req, int fork_id) { } - /// Called with all responses, except 100 Trying, received on the transaction. /// If a transport error or transaction timeout occurs on a downstream leg, /// this method is called with a 408 response. /// /// Note: 100 Trying responses are handled by the on_rx_trying method. /// - /// @param rsp - The received request. + /// @param rsp - The received response. /// @param fork_id - The identity of the downstream fork on which /// the response was received. virtual void on_rx_response(pjsip_msg* rsp, int fork_id) { send_response(rsp); } @@ -345,13 +336,8 @@ class SproutletTsx /// These responses are only sent by the wrappers, so we shouldn't call /// ever call send_response on them. /// - /// @param rsp - The received request. - virtual void on_rx_trying(pjsip_msg* rsp) {} - - /// Called when a response has been transmitted on the transaction. - /// - /// @param rsp - The transmitted response. - virtual void on_tx_response(pjsip_msg* rsp) { } + /// @param rsp - The received response. + virtual void on_rx_trying(pjsip_msg* rsp, int fork_id) {} /// Called if the original request is cancelled (either by a received /// CANCEL request, an error on the inbound transport or a transaction @@ -366,6 +352,33 @@ class SproutletTsx /// was triggered by an error or timeout. virtual void on_rx_cancel(int status_code, pjsip_msg* cancel_req) {} + /// Called when a request is received by the sproutlet wrapper. + /// + /// @param req - The received request. + virtual void obs_rx_request(pjsip_msg* req) {}; + + /// Called when a response is received by the sproutlet wrapper. + /// + /// @param rsp - The received response. + /// @param fork_id - The identity of the downstream fork on which the + /// response was received. + virtual void obs_rx_response(pjsip_msg* rsp, int fork_id) {}; + + /// Called when a request has been transmitted on the transaction by the + /// wrapper. This is usually because the service has previously called + /// forward_request() with the request message. + /// + /// @param req - The transmitted request + /// @param fork_id - The identity of the downstream fork on which the + /// request was sent. + virtual void obs_tx_request(pjsip_msg* req, int fork_id) {} + + /// Called when a response has been transmitted on the transaction by the + /// wrapper. + /// + /// @param rsp - The transmitted response. + virtual void obs_tx_response(pjsip_msg* rsp) {} + /// Called when a timer programmed by the SproutletTsx expires. /// /// @param context - The context parameter specified when the timer diff --git a/src/bgcfsproutlet.cpp b/src/bgcfsproutlet.cpp index b4aedaf51..e782c2936 100644 --- a/src/bgcfsproutlet.cpp +++ b/src/bgcfsproutlet.cpp @@ -253,7 +253,7 @@ void BGCFSproutletTsx::on_rx_initial_request(pjsip_msg* req) } -void BGCFSproutletTsx::on_tx_request(pjsip_msg* req, int fork_id) +void BGCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) { if (_acr != NULL) { @@ -279,7 +279,7 @@ void BGCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void BGCFSproutletTsx::on_tx_response(pjsip_msg* rsp) +void BGCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) { if (_acr != NULL) { diff --git a/src/icscfsproutlet.cpp b/src/icscfsproutlet.cpp index 8edc0c0e9..749b285b2 100644 --- a/src/icscfsproutlet.cpp +++ b/src/icscfsproutlet.cpp @@ -297,7 +297,7 @@ void ICSCFSproutletRegTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void ICSCFSproutletRegTsx::on_tx_request(pjsip_msg* req, int fork_id) +void ICSCFSproutletRegTsx::obs_tx_request(pjsip_msg* req, int fork_id) { if (_acr != NULL) { @@ -393,7 +393,7 @@ void ICSCFSproutletRegTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void ICSCFSproutletRegTsx::on_tx_response(pjsip_msg* rsp) +void ICSCFSproutletRegTsx::obs_tx_response(pjsip_msg* rsp) { if (_acr != NULL) { @@ -710,7 +710,7 @@ void ICSCFSproutletTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void ICSCFSproutletTsx::on_tx_request(pjsip_msg* req, int fork_id) +void ICSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) { if (_acr != NULL) { @@ -799,7 +799,7 @@ void ICSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void ICSCFSproutletTsx::on_tx_response(pjsip_msg* rsp) +void ICSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) { if (_acr != NULL) { diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index 4670a1c65..7dd0510bf 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -573,7 +573,7 @@ void SCSCFSproutletTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void SCSCFSproutletTsx::on_tx_request(pjsip_msg* req, int fork_id) +void SCSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) { ACR* acr = get_acr(); if (acr) @@ -699,13 +699,13 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp) +void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp, int fork_id) { common_response_processing(rsp); } -void SCSCFSproutletTsx::on_tx_response(pjsip_msg* rsp) +void SCSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) { ACR* acr = get_acr(); if (acr != NULL) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 8a8922016..34fdaf106 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1589,6 +1589,9 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) log_inter_sproutlet(req, true); } + // Notify the sproutlet that an a request has been received. + _sproutlet_tsx->obs_rx_request(req->msg); + // Keep an immutable reference to the request. _req = req; @@ -1643,6 +1646,9 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) log_inter_sproutlet(rsp, false); } + // Notify the sproutlet that an a response has been received. + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id); + register_tdata(rsp); int status_code = rsp->msg->line.status.code; if ((PJSIP_IS_STATUS_IN_CLASS(status_code, 100)) && @@ -1684,7 +1690,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) if (status_code == PJSIP_SC_TRYING) { - _sproutlet_tsx->on_rx_trying(rsp->msg); + _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); } else { @@ -1888,6 +1894,8 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) return; } + // LCOV_EXCL_START - Sproutlets don't forward 100 Trying responses so we + // should never hit this code. But let's keep it here just in case. if (status_code == 100) { // We will already have sent a locally generated 100 Trying response, so @@ -1897,6 +1905,7 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) pjsip_tx_data_dec_ref(rsp); return; } + // LCOV_EXCL_STOP if ((status_code > 100) && (status_code < 199)) @@ -1978,7 +1987,7 @@ void SproutletWrapper::tx_request(pjsip_tx_data* req, int fork_id) } // Notify the sproutlet that the request is being sent downstream. - _sproutlet_tsx->on_tx_request(req->msg, fork_id); + _sproutlet_tsx->obs_tx_request(req->msg, fork_id); // Forward the request downstream. deregister_tdata(req); @@ -1987,9 +1996,6 @@ void SproutletWrapper::tx_request(pjsip_tx_data* req, int fork_id) void SproutletWrapper::tx_response(pjsip_tx_data* rsp) { - // Notify the sproutlet that the response is being sent upstream. - _sproutlet_tsx->on_tx_response(rsp->msg); - if (rsp->msg->line.status.code >= PJSIP_SC_OK) { _complete = true; @@ -2010,6 +2016,9 @@ void SproutletWrapper::tx_response(pjsip_tx_data* rsp) } } + // Notify the sproutlet that the response is being sent upstream. + _sproutlet_tsx->obs_tx_response(rsp->msg); + // Forward the response upstream. deregister_tdata(rsp); _proxy_tsx->tx_response(this, rsp); diff --git a/src/ut/mock_sproutlet.h b/src/ut/mock_sproutlet.h index 32aba09c6..cd73e8111 100644 --- a/src/ut/mock_sproutlet.h +++ b/src/ut/mock_sproutlet.h @@ -72,9 +72,9 @@ class MockSproutletTsx : public SproutletTsx MOCK_METHOD1(on_rx_initial_request, void(pjsip_msg*)); MOCK_METHOD1(on_rx_in_dialog_request, void(pjsip_msg*)); - MOCK_METHOD2(on_tx_request, void(pjsip_msg*, int)); + MOCK_METHOD2(obs_tx_request, void(pjsip_msg*, int)); MOCK_METHOD2(on_rx_response, void(pjsip_msg*, int)); - MOCK_METHOD1(on_tx_response, void(pjsip_msg*)); + MOCK_METHOD1(obs_tx_response, void(pjsip_msg*)); MOCK_METHOD2(on_rx_cancel, void(int, pjsip_msg*)); MOCK_METHOD1(on_timer_expiry, void(void*)); }; From dc746b1d72bacd8d3fa4f75610e6c03c75fb84c7 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Fri, 24 Feb 2017 11:17:51 +0000 Subject: [PATCH 03/17] Send 200 OK to CANCEL and negative ACKs between sproutlets --- include/basicproxy.h | 5 + include/pjutils.h | 4 + include/sproutletproxy.h | 2 +- modules/pjsip | 2 +- src/basicproxy.cpp | 10 ++ src/pjutils.cpp | 20 +++- src/sproutletproxy.cpp | 223 ++++++++++++++++++++++++--------------- 7 files changed, 180 insertions(+), 86 deletions(-) diff --git a/include/basicproxy.h b/include/basicproxy.h index bbc24e702..39b7cd1d2 100644 --- a/include/basicproxy.h +++ b/include/basicproxy.h @@ -254,6 +254,11 @@ class BasicProxy /// initialised to a 408 Request Timeout response. pjsip_tx_data* _final_rsp; + /// If flag is set, any ACKs generated in the sproutlet wrapper will be + /// absorbed. This is so that we do not send ACKs to negative responses off + /// the box. + bool _absorb_acks; + bool _pending_destroy; int _context_count; diff --git a/include/pjutils.h b/include/pjutils.h index cb47d68b4..fc61fd6ae 100644 --- a/include/pjutils.h +++ b/include/pjutils.h @@ -179,6 +179,10 @@ pjsip_tx_data* create_cancel(pjsip_endpoint* endpt, pjsip_tx_data* tdata, int reason_code); +pjsip_tx_data* create_ack(pjsip_endpoint* endpt, + pjsip_tx_data* original_request, + pjsip_msg* rsp); + void resolve(const std::string& name, int port, int transport, diff --git a/include/sproutletproxy.h b/include/sproutletproxy.h index 35c46dc96..d23797b22 100644 --- a/include/sproutletproxy.h +++ b/include/sproutletproxy.h @@ -318,7 +318,7 @@ class SproutletWrapper : public SproutletTsxHelper private: void rx_request(pjsip_tx_data* req); - void rx_response(pjsip_tx_data* rsp, int fork_id); + void rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp); void rx_cancel(pjsip_tx_data* cancel); void rx_error(int status_code); void rx_fork_error(pjsip_event_id_e event, int fork_id); diff --git a/modules/pjsip b/modules/pjsip index c8845f684..a33a3b92a 160000 --- a/modules/pjsip +++ b/modules/pjsip @@ -1 +1 @@ -Subproject commit c8845f6845d53860861d198010b532f8d7a5bd24 +Subproject commit a33a3b92a1b7076a7e3eb314e55a786e20c845f6 diff --git a/src/basicproxy.cpp b/src/basicproxy.cpp index 6d0aaf9ce..bae617f49 100644 --- a/src/basicproxy.cpp +++ b/src/basicproxy.cpp @@ -458,6 +458,7 @@ BasicProxy::UASTsx::UASTsx(BasicProxy* proxy) : _pending_sends(0), _pending_responses(0), _final_rsp(NULL), + _absorb_acks(false), _pending_destroy(false), _context_count(0) { @@ -622,12 +623,21 @@ pj_status_t BasicProxy::UASTsx::init(pjsip_rx_data* rdata) (PJSIP_T2_TIMEOUT - PJSIP_T1_TIMEOUT) % 1000 }; pjsip_endpt_schedule_timer(stack_data.endpt, &(_trying_timer), &delay); } + + // Sproutlets may generate local ACKs to negative responses, we should not + // send these off the box, so set a flag to absorb ACKs that are received + // by the sproutlet wrappers. + _absorb_acks = true; } else { // ACK will be forwarded statelessly, so we don't need a PJSIP transaction. // Enter the context of this object so the context count gets incremented. enter_context(); + + // For ACK transactions, we should not absorb ACKS that are generated by + // the sproutlets. + _absorb_acks = false; } return PJ_SUCCESS; diff --git a/src/pjutils.cpp b/src/pjutils.cpp index 9ac2dffc4..401fa8966 100644 --- a/src/pjutils.cpp +++ b/src/pjutils.cpp @@ -906,6 +906,24 @@ pjsip_tx_data* PJUtils::create_cancel(pjsip_endpoint* endpt, return cancel; } +pjsip_tx_data* PJUtils::create_ack(pjsip_endpoint* endpt, + pjsip_tx_data* original_request, + pjsip_msg* rsp) +{ + pjsip_tx_data* ack; + pj_status_t status = pjsip_create_ack(endpt, + original_request, + rsp, + &ack); + + if (status != PJ_SUCCESS) + { + return NULL; + } + + return ack; +} + /// Resolves a destination. void PJUtils::resolve(const std::string& name, int port, @@ -1683,7 +1701,7 @@ void PJUtils::mark_sas_call_branch_ids(const SAS::TrailId trail, pjsip_cid_hdr* ((msg->line.req.method.id == PJSIP_REGISTER_METHOD) || (pjsip_method_cmp(&msg->line.req.method, pjsip_get_subscribe_method()) == 0) || (pjsip_method_cmp(&msg->line.req.method, pjsip_get_notify_method()) == 0))); - + if (cid_hdr != NULL) { TRC_DEBUG("Logging SAS Call-ID marker, Call-ID %.*s", cid_hdr->id.slen, cid_hdr->id.ptr); diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 34fdaf106..6215ada28 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -625,9 +625,6 @@ void SproutletProxy::UASTsx::process_cancel_request(pjsip_rx_data* rdata) // Pass the CANCEL to the Sproutlet at the root of the tree. pjsip_tx_data* tdata = PJUtils::clone_msg(stack_data.endpt, rdata); _root->rx_cancel(tdata); - - // Schedule any requests generated by the Sproutlet. - schedule_requests(); } } @@ -660,7 +657,7 @@ void SproutletProxy::UASTsx::on_new_client_response(UACTsx* uac_tsx, _umap.erase(i); } - upstream_sproutlet->rx_response(rsp, upstream_fork); + upstream_sproutlet->rx_response(rsp, upstream_fork, true); // Schedule any requests generated by the Sproutlet. schedule_requests(); @@ -824,7 +821,7 @@ void SproutletProxy::UASTsx::schedule_requests() if (status == PJ_SUCCESS) { // Pass the response back to the Sproutlet. - req.upstream.first->rx_response(rsp, req.upstream.second); + req.upstream.first->rx_response(rsp, req.upstream.second, false); } } else @@ -870,7 +867,7 @@ void SproutletProxy::UASTsx::schedule_requests() &trying); if (status == PJ_SUCCESS) { - req.upstream.first->rx_response(trying, req.upstream.second); + req.upstream.first->rx_response(trying, req.upstream.second, false); } } @@ -1017,13 +1014,16 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, SproutletWrapper* upstream = i->second.first; int fork_id = i->second.second; - if (rsp->msg->line.status.code >= PJSIP_SC_OK) + // We only treat this like a final response if the code is >= 200 and + // this is not a 200 OK to a CANCEL. + if ((rsp->msg->line.status.code >= PJSIP_SC_OK) && + (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id != PJSIP_CANCEL_METHOD)) { // Final response, so break the linkage between the Sproutlets. _dmap_sproutlet.erase(i->second); _umap.erase(i); } - upstream->rx_response(rsp, fork_id); + upstream->rx_response(rsp, fork_id, false); } else { @@ -1592,46 +1592,57 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) // Notify the sproutlet that an a request has been received. _sproutlet_tsx->obs_rx_request(req->msg); - // Keep an immutable reference to the request. - _req = req; - - // Decrement Max-Forwards if present. - pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) - pjsip_msg_find_hdr(req->msg, PJSIP_H_MAX_FORWARDS, NULL); - if (mf_hdr != NULL) - { - --mf_hdr->ivalue; - } - - // Clone the request to get a mutable copy to pass to the Sproutlet. - pjsip_msg* clone = original_request(); - if (clone == NULL) + // If this is an ACK request and we have already decided to absorb ACK + // requests in the sproutlets, then release our reference to this message. + if ((req->msg->line.req.method.id == PJSIP_ACK_METHOD) && + (_proxy_tsx->_absorb_acks)) { - // @TODO - } - - if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) - { - TRC_VERBOSE("%s pass initial request %s to Sproutlet", - _id.c_str(), msg_info(clone)); - _sproutlet_tsx->on_rx_initial_request(clone); + TRC_DEBUG("Absorb ACK to negative response"); + pjsip_tx_data_dec_ref(req); } else { - TRC_VERBOSE("%s pass in dialog request %s to Sproutlet", - _id.c_str(), msg_info(clone)); - _sproutlet_tsx->on_rx_in_dialog_request(clone); - } + // Keep an immutable reference to the request. + _req = req; + + // Decrement Max-Forwards if present. + pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) + pjsip_msg_find_hdr(req->msg, PJSIP_H_MAX_FORWARDS, NULL); + if (mf_hdr != NULL) + { + --mf_hdr->ivalue; + } + + // Clone the request to get a mutable copy to pass to the Sproutlet. + pjsip_msg* clone = original_request(); + if (clone == NULL) + { + // @TODO + } - // We consider an ACK transaction to be complete immediately after the - // sproutlet's actions have been processed, regardless of whether the - // sproutlet forwarded the ACK (some sproutlets are unable to in certain - // situations). - bool complete_after_actions = (req->msg->line.req.method.id == PJSIP_ACK_METHOD); - process_actions(complete_after_actions); + if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) + { + TRC_VERBOSE("%s pass initial request %s to Sproutlet", + _id.c_str(), msg_info(clone)); + _sproutlet_tsx->on_rx_initial_request(clone); + } + else + { + TRC_VERBOSE("%s pass in dialog request %s to Sproutlet", + _id.c_str(), msg_info(clone)); + _sproutlet_tsx->on_rx_in_dialog_request(clone); + } + + // We consider an ACK transaction to be complete immediately after the + // sproutlet's actions have been processed, regardless of whether the + // sproutlet forwarded the ACK (some sproutlets are unable to in certain + // situations). + bool complete_after_actions = (req->msg->line.req.method.id == PJSIP_ACK_METHOD); + process_actions(complete_after_actions); + } } -void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) +void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp) { // SAS log the start of processing by this sproutlet SAS::Event event(trail(), SASEvent::BEGIN_SPROUTLET_RSP, 0); @@ -1646,65 +1657,108 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) log_inter_sproutlet(rsp, false); } + // If this is a negative response, send an immediate ACK response. We don't + // do this id this response has been passed to us directly from a UACTsx since + // PJSIP will have sent and ACK already. + if ((rsp->msg->line.status.code >= 300) && + (!client_rsp)) + { + TRC_DEBUG("Send immediate ACK to negative response"); + pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, + _forks[fork_id].req, + rsp->msg); + ++_pending_sends; + tx_request(ack, fork_id); + _proxy_tsx->schedule_requests(); + } + // Notify the sproutlet that an a response has been received. _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id); - register_tdata(rsp); - int status_code = rsp->msg->line.status.code; - if ((PJSIP_IS_STATUS_IN_CLASS(status_code, 100)) && - (_forks[fork_id].state.tsx_state == PJSIP_TSX_STATE_CALLING)) + // If this is a 200 OK to a CANCEL, we don't want to do anything with it, so + // just drop our reference to it. + if (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id == PJSIP_CANCEL_METHOD) { - // Provisional response on fork still in calling state, so move to - // proceeding state. - _forks[fork_id].state.tsx_state = PJSIP_TSX_STATE_PROCEEDING; - TRC_VERBOSE("%s received provisional response %s on fork %d, state = %s", - _id.c_str(), pjsip_tx_data_get_info(rsp), - fork_id, pjsip_tsx_state_str(_forks[fork_id].state.tsx_state)); + pjsip_tx_data_dec_ref(rsp); } - else if (status_code >= PJSIP_SC_OK) + else { - // Final response, so mark the fork as completed and decrement the number - // of pending responses. - _forks[fork_id].state.tsx_state = PJSIP_TSX_STATE_TERMINATED; - pjsip_tx_data_dec_ref(_forks[fork_id].req); - _forks[fork_id].req = NULL; - TRC_VERBOSE("%s received final response %s on fork %d, state = %s", - _id.c_str(), pjsip_tx_data_get_info(rsp), - fork_id, pjsip_tsx_state_str(_forks[fork_id].state.tsx_state)); - --_pending_responses; - - if ((_sproutlet != NULL) && - (_sproutlet->_outgoing_sip_transactions_tbl != NULL)) + register_tdata(rsp); + int status_code = rsp->msg->line.status.code; + if ((PJSIP_IS_STATUS_IN_CLASS(status_code, 100)) && + (_forks[fork_id].state.tsx_state == PJSIP_TSX_STATE_CALLING)) { - // Update SNMP SIP transactions statistics for the Sproutlet. - if (status_code >= 200 && status_code < 300) - { - _sproutlet->_outgoing_sip_transactions_tbl->increment_successes(_req_type); - } - else + // Provisional response on fork still in calling state, so move to + // proceeding state. + _forks[fork_id].state.tsx_state = PJSIP_TSX_STATE_PROCEEDING; + TRC_VERBOSE("%s received provisional response %s on fork %d, state = %s", + _id.c_str(), pjsip_tx_data_get_info(rsp), + fork_id, pjsip_tsx_state_str(_forks[fork_id].state.tsx_state)); + } + else if (status_code >= PJSIP_SC_OK) + { + // Final response, so mark the fork as completed and decrement the number + // of pending responses. + _forks[fork_id].state.tsx_state = PJSIP_TSX_STATE_TERMINATED; + pjsip_tx_data_dec_ref(_forks[fork_id].req); + _forks[fork_id].req = NULL; + TRC_VERBOSE("%s received final response %s on fork %d, state = %s", + _id.c_str(), pjsip_tx_data_get_info(rsp), + fork_id, pjsip_tsx_state_str(_forks[fork_id].state.tsx_state)); + --_pending_responses; + + if ((_sproutlet != NULL) && + (_sproutlet->_outgoing_sip_transactions_tbl != NULL)) { - _sproutlet->_outgoing_sip_transactions_tbl->increment_failures(_req_type); + // Update SNMP SIP transactions statistics for the Sproutlet. + if (status_code >= 200 && status_code < 300) + { + _sproutlet->_outgoing_sip_transactions_tbl->increment_successes(_req_type); + } + else + { + _sproutlet->_outgoing_sip_transactions_tbl->increment_failures(_req_type); + } } } - } - if (status_code == PJSIP_SC_TRYING) - { - _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); - } - else - { - _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); + if (status_code == PJSIP_SC_TRYING) + { + _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); + } + else + { + _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); + } + process_actions(false); } - - process_actions(false); } void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) { TRC_VERBOSE("%s received CANCEL request", _id.c_str()); + + // If this isn't the root sproutlet, send an immediate 200 OK to the CANCEL. + if (_proxy_tsx->_root != this) + { + pjsip_tx_data* ok; + pj_status_t status = PJUtils::create_response(stack_data.endpt, + cancel, + PJSIP_SC_OK, + NULL, + &ok); + + if (status == PJ_SUCCESS) + { + tx_response(ok); + } + } + + // Notify the sproutlet that a request has been received. + _sproutlet_tsx->obs_rx_request(cancel->msg); + _sproutlet_tsx->on_rx_cancel(PJSIP_SC_REQUEST_TERMINATED, - cancel->msg); + cancel->msg); pjsip_tx_data_dec_ref(cancel); cancel_pending_forks(); process_actions(false); @@ -1996,7 +2050,10 @@ void SproutletWrapper::tx_request(pjsip_tx_data* req, int fork_id) void SproutletWrapper::tx_response(pjsip_tx_data* rsp) { - if (rsp->msg->line.status.code >= PJSIP_SC_OK) + // We only treat this as a final response if the status code is >= 200 and + // this is not a 200 OK to a CANCEL. + if ((rsp->msg->line.status.code >= PJSIP_SC_OK) && + (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id != PJSIP_CANCEL_METHOD)) { _complete = true; cancel_pending_forks(); From 59be2683d2d444b2aeb2f11b88464dc4f105436a Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Fri, 24 Feb 2017 23:40:18 +0000 Subject: [PATCH 04/17] Create ACK from msg --- include/pjutils.h | 2 +- src/pjutils.cpp | 10 +++++----- src/sproutletproxy.cpp | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/pjutils.h b/include/pjutils.h index fc61fd6ae..b1fbd3655 100644 --- a/include/pjutils.h +++ b/include/pjutils.h @@ -180,7 +180,7 @@ pjsip_tx_data* create_cancel(pjsip_endpoint* endpt, int reason_code); pjsip_tx_data* create_ack(pjsip_endpoint* endpt, - pjsip_tx_data* original_request, + pjsip_msg* req, pjsip_msg* rsp); void resolve(const std::string& name, diff --git a/src/pjutils.cpp b/src/pjutils.cpp index 401fa8966..2d586e3fa 100644 --- a/src/pjutils.cpp +++ b/src/pjutils.cpp @@ -907,14 +907,14 @@ pjsip_tx_data* PJUtils::create_cancel(pjsip_endpoint* endpt, } pjsip_tx_data* PJUtils::create_ack(pjsip_endpoint* endpt, - pjsip_tx_data* original_request, + pjsip_msg* req, pjsip_msg* rsp) { pjsip_tx_data* ack; - pj_status_t status = pjsip_create_ack(endpt, - original_request, - rsp, - &ack); + pj_status_t status = pjsip_endpt_create_ack_from_msgs(endpt, + req, + rsp, + &ack); if (status != PJ_SUCCESS) { diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 6215ada28..4f0a09af4 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1665,7 +1665,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ { TRC_DEBUG("Send immediate ACK to negative response"); pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, - _forks[fork_id].req, + _forks[fork_id].req->msg, rsp->msg); ++_pending_sends; tx_request(ack, fork_id); From 4894d65f857b146c70e3e925d66c2df344e4c5d9 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Fri, 24 Feb 2017 23:41:49 +0000 Subject: [PATCH 05/17] Decrement max-forwards after cloning the message --- src/sproutletproxy.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 4f0a09af4..ac9c1649d 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1605,14 +1605,6 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) // Keep an immutable reference to the request. _req = req; - // Decrement Max-Forwards if present. - pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) - pjsip_msg_find_hdr(req->msg, PJSIP_H_MAX_FORWARDS, NULL); - if (mf_hdr != NULL) - { - --mf_hdr->ivalue; - } - // Clone the request to get a mutable copy to pass to the Sproutlet. pjsip_msg* clone = original_request(); if (clone == NULL) @@ -1620,6 +1612,14 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) // @TODO } + // Decrement Max-Forwards if present. + pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) + pjsip_msg_find_hdr(clone, PJSIP_H_MAX_FORWARDS, NULL); + if (mf_hdr != NULL) + { + --mf_hdr->ivalue; + } + if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) { TRC_VERBOSE("%s pass initial request %s to Sproutlet", From 92fb6a52a31e86f7b2a74f301526b1f418becce1 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Sat, 25 Feb 2017 12:06:35 +0000 Subject: [PATCH 06/17] Fix memory leak --- src/sproutletproxy.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index ac9c1649d..db2273188 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1593,12 +1593,13 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) _sproutlet_tsx->obs_rx_request(req->msg); // If this is an ACK request and we have already decided to absorb ACK - // requests in the sproutlets, then release our reference to this message. + // requests in the sproutlets, then just go to process actions which will + // destory this wrapper. if ((req->msg->line.req.method.id == PJSIP_ACK_METHOD) && (_proxy_tsx->_absorb_acks)) { TRC_DEBUG("Absorb ACK to negative response"); - pjsip_tx_data_dec_ref(req); + process_actions(true); } else { From da0d64487855e800a4ef6abac77f9f030937c967 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Mon, 27 Feb 2017 08:46:53 +0000 Subject: [PATCH 07/17] Take latest PJSIP --- modules/pjsip | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/pjsip b/modules/pjsip index a33a3b92a..3ab43b6c1 160000 --- a/modules/pjsip +++ b/modules/pjsip @@ -1 +1 @@ -Subproject commit a33a3b92a1b7076a7e3eb314e55a786e20c845f6 +Subproject commit 3ab43b6c1ea9db7c17a585e1357cadc9356240e3 From 8f06221cea816c2f7c4596637b1c1393acb91ae2 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Tue, 28 Feb 2017 14:34:13 +0000 Subject: [PATCH 08/17] Markups --- include/basicproxy.h | 5 -- include/scscfsproutlet.h | 6 +- include/sproutlet.h | 6 +- include/sproutletproxy.h | 10 +++ src/basicproxy.cpp | 10 --- src/icscfsproutlet.cpp | 6 +- src/scscfsproutlet.cpp | 14 ++-- src/sproutletproxy.cpp | 119 ++++++++++++++++++++++++--------- src/ut/mock_sproutlet.h | 2 + src/ut/sproutletproxy_test.cpp | 7 ++ 10 files changed, 125 insertions(+), 60 deletions(-) diff --git a/include/basicproxy.h b/include/basicproxy.h index 39b7cd1d2..bbc24e702 100644 --- a/include/basicproxy.h +++ b/include/basicproxy.h @@ -254,11 +254,6 @@ class BasicProxy /// initialised to a 408 Request Timeout response. pjsip_tx_data* _final_rsp; - /// If flag is set, any ACKs generated in the sproutlet wrapper will be - /// absorbed. This is so that we do not send ACKs to negative responses off - /// the box. - bool _absorb_acks; - bool _pending_destroy; int _context_count; diff --git a/include/scscfsproutlet.h b/include/scscfsproutlet.h index 46ba6b901..f96b52d82 100644 --- a/include/scscfsproutlet.h +++ b/include/scscfsproutlet.h @@ -366,9 +366,9 @@ class SCSCFSproutletTsx : public SproutletTsx /// @param sip_code - The reported SIP return code std::string fork_failure_reason_as_string(int fork_id, int sip_code); - /// Do common processing that we do for all responses. This is called by - /// on_rx_response and on_rx_trying. - void common_response_processing(pjsip_msg* rsp); + void acr_handle_response(pjsip_msg* rsp); + + void cancel_liveness_timer(); /// Pointer to the parent SCSCFSproutlet object - used for various operations /// that require access to global configuration or services. diff --git a/include/sproutlet.h b/include/sproutlet.h index 31d8916d8..40e034cec 100644 --- a/include/sproutlet.h +++ b/include/sproutlet.h @@ -332,9 +332,9 @@ class SproutletTsx /// the response was received. virtual void on_rx_response(pjsip_msg* rsp, int fork_id) { send_response(rsp); } - /// Called if a 100 Trying response is received on the transaction. If the - /// These responses are only sent by the wrappers, so we shouldn't call - /// ever call send_response on them. + /// This is a notification that a 100 Trying has been received. The sproutlet + /// proxy handles these automatically. The Sproutlet Tsx should not forward + /// the response on (i.e. should not call send_request). /// /// @param rsp - The received response. virtual void on_rx_trying(pjsip_msg* rsp, int fork_id) {} diff --git a/include/sproutletproxy.h b/include/sproutletproxy.h index d23797b22..ff02c119b 100644 --- a/include/sproutletproxy.h +++ b/include/sproutletproxy.h @@ -191,6 +191,10 @@ class SproutletProxy : public BasicProxy int fork_id, pjsip_tx_data* cancel); + void tx_negative_ack(SproutletWrapper* sproutlet, + int fork_id, + pjsip_tx_data* ack); + /// Gets the next target Sproutlet for the message by analysing the top /// Route header. Sproutlet* target_sproutlet(pjsip_msg* msg, @@ -252,6 +256,11 @@ class SproutletProxy : public BasicProxy /// The UASTsx will persist while there are pending timers. std::set _pending_timers; + /// If flag is set, any ACKs generated in the sproutlet wrapper will be + /// absorbed. This is so that we do not send ACKs to negative responses off + /// the box. + bool _absorb_acks; + friend class SproutletWrapper; }; @@ -331,6 +340,7 @@ class SproutletWrapper : public SproutletTsxHelper void tx_request(pjsip_tx_data* req, int fork_id); void tx_response(pjsip_tx_data* rsp); void tx_cancel(int fork_id); + void tx_negative_ack(pjsip_tx_data* rsp, int fork_id); int compare_sip_sc(int sc1, int sc2); bool is_uri_local(const pjsip_uri*) const; void log_inter_sproutlet(pjsip_tx_data* tdata, bool downstream); diff --git a/src/basicproxy.cpp b/src/basicproxy.cpp index bae617f49..6d0aaf9ce 100644 --- a/src/basicproxy.cpp +++ b/src/basicproxy.cpp @@ -458,7 +458,6 @@ BasicProxy::UASTsx::UASTsx(BasicProxy* proxy) : _pending_sends(0), _pending_responses(0), _final_rsp(NULL), - _absorb_acks(false), _pending_destroy(false), _context_count(0) { @@ -623,21 +622,12 @@ pj_status_t BasicProxy::UASTsx::init(pjsip_rx_data* rdata) (PJSIP_T2_TIMEOUT - PJSIP_T1_TIMEOUT) % 1000 }; pjsip_endpt_schedule_timer(stack_data.endpt, &(_trying_timer), &delay); } - - // Sproutlets may generate local ACKs to negative responses, we should not - // send these off the box, so set a flag to absorb ACKs that are received - // by the sproutlet wrappers. - _absorb_acks = true; } else { // ACK will be forwarded statelessly, so we don't need a PJSIP transaction. // Enter the context of this object so the context count gets incremented. enter_context(); - - // For ACK transactions, we should not absorb ACKS that are generated by - // the sproutlets. - _absorb_acks = false; } return PJ_SUCCESS; diff --git a/src/icscfsproutlet.cpp b/src/icscfsproutlet.cpp index 749b285b2..32006131c 100644 --- a/src/icscfsproutlet.cpp +++ b/src/icscfsproutlet.cpp @@ -813,12 +813,14 @@ void ICSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) // Check if this is a terminating INVITE. If it is then check whether we need // to update our session establishment stats. We consider the session to be // set up as soon as we receive a final response OR a 180 Ringing (per TS - // 32.409). + // 32.409). We do not consider responses to CANCEL requests as final + // responses. if (!_originating && (_req_type == PJSIP_INVITE_METHOD) && !_session_set_up && ((rsp_status == PJSIP_SC_RINGING) || - !PJSIP_IS_STATUS_IN_CLASS(rsp_status, 100))) + !PJSIP_IS_STATUS_IN_CLASS(rsp_status, 100)) && + (PJSIP_MSG_CSEQ_HDR(rsp)->method.id != PJSIP_CANCEL_METHOD)) { // Session is now set up. _session_set_up = true; diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index def0d313d..15f7875a2 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -604,7 +604,8 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) _se_helper.process_response(rsp, get_pool(rsp), trail()); } - common_response_processing(rsp); + acr_handle_response(rsp); + cancel_liveness_timer(); int st_code = rsp->line.status.code; @@ -711,7 +712,8 @@ void SCSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp, int fork_id) { - common_response_processing(rsp); + acr_handle_response(rsp); + cancel_liveness_timer(); } @@ -733,7 +735,8 @@ void SCSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) pjsip_status_code st_code = (pjsip_status_code)rsp->line.status.code; if (_record_session_setup_time && ((st_code == PJSIP_SC_RINGING) || - PJSIP_IS_STATUS_IN_CLASS(st_code, 200))) + ((PJSIP_IS_STATUS_IN_CLASS(st_code, 200)) && + (PJSIP_MSG_CSEQ_HDR(rsp)->method.id != PJSIP_CANCEL_METHOD)))) { _scscf->track_session_setup_time(_tsx_start_time_usec, _video_call); _record_session_setup_time = false; @@ -2169,7 +2172,7 @@ std::string SCSCFSproutletTsx::fork_failure_reason_as_string(int fork_id, int si return reason; } -void SCSCFSproutletTsx::common_response_processing(pjsip_msg* rsp) +void SCSCFSproutletTsx::acr_handle_response(pjsip_msg* rsp) { // Pass the received response to the ACR. // @TODO - timestamp from response??? @@ -2180,7 +2183,10 @@ void SCSCFSproutletTsx::common_response_processing(pjsip_msg* rsp) acr->rx_response(rsp); acr->unlock(); } +} +void SCSCFSproutletTsx::cancel_liveness_timer() +{ if (_liveness_timer != 0) { // The liveness timer is running on this request, so cancel it. diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index db2273188..03942b8eb 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -545,6 +545,20 @@ pj_status_t SproutletProxy::UASTsx::init(pjsip_rx_data* rdata) // Do the BasicProxy initialization first. pj_status_t status = BasicProxy::UASTsx::init(rdata); + if (rdata->msg_info.msg->line.req.method.id != PJSIP_ACK_METHOD) + { + // Sproutlets may generate local ACKs to negative responses, we should not + // send these off the box, so set a flag to absorb ACKs that are received + // by the sproutlet wrappers. + _absorb_acks = true; + } + else + { + // For ACK transactions, we should not absorb ACKS that are generated by + // the sproutlets. + _absorb_acks = false; + } + if (status == PJ_SUCCESS) { // Locate the target Sproutlet for the request, and create the helper and @@ -977,6 +991,14 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, { if (downstream == _root) { + // If this is the root sproutlet in the tree, drop any 200 OK to CANCEL + // messages that the sproutlets generated. + if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) + { + pjsip_tx_data_dec_ref(rsp); + return; + } + // This is the root sproutlet in the tree, so send the response on the // UAS transaction. if (_tsx != NULL) @@ -1014,12 +1036,14 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, SproutletWrapper* upstream = i->second.first; int fork_id = i->second.second; - // We only treat this like a final response if the code is >= 200 and - // this is not a 200 OK to a CANCEL. + // We do not treat 200 OK to CANCEL as a final response. if ((rsp->msg->line.status.code >= PJSIP_SC_OK) && - (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id != PJSIP_CANCEL_METHOD)) + (rsp->msg->line.status.code < 300) && + (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_CANCEL_METHOD)) { - // Final response, so break the linkage between the Sproutlets. + // Final 2xx response, so break the linkage between the Sproutlets. + // For 3xx-6xx responses, the linkage between the sproutlets will be + // broken once the ACK to the response has been sent. _dmap_sproutlet.erase(i->second); _umap.erase(i); } @@ -1074,6 +1098,34 @@ void SproutletProxy::UASTsx::tx_cancel(SproutletWrapper* upstream, } +void SproutletProxy::UASTsx::tx_negative_ack(SproutletWrapper* upstream, + int fork_id, + pjsip_tx_data* ack) +{ + TRC_DEBUG("Process negative ACK from %s on fork %d", + upstream->service_name().c_str(), fork_id); + DMap::iterator i = + _dmap_sproutlet.find(std::make_pair(upstream, fork_id)); + if (i != _dmap_sproutlet.end()) + { + // Pass the ACK request to the downstream Sproutlet. + SproutletWrapper* downstream = i->second; + TRC_DEBUG("Route negative ACK to %s", downstream->service_name().c_str()); + downstream->rx_request(ack); + + // This is an ACK to a final response so disconnect the sproutlets. + _dmap_sproutlet.erase(std::make_pair(upstream, fork_id)); + UMap::iterator j = _umap.find((void*)downstream); + _umap.erase(j); + } + else + { + // We don't want to route this externally, so drop our reference to it. + pjsip_tx_data_dec_ref(ack); + } +} + + /// Checks to see if the UASTsx can be destroyed. It is only safe to destroy /// the UASTsx when all the Sproutlet's have completed their processing, which /// only occurs when all the linkages are broken. @@ -1593,13 +1645,12 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) _sproutlet_tsx->obs_rx_request(req->msg); // If this is an ACK request and we have already decided to absorb ACK - // requests in the sproutlets, then just go to process actions which will - // destory this wrapper. + // requests in the sproutlets, drop our reference to this request. if ((req->msg->line.req.method.id == PJSIP_ACK_METHOD) && (_proxy_tsx->_absorb_acks)) { TRC_DEBUG("Absorb ACK to negative response"); - process_actions(true); + pjsip_tx_data_dec_ref(req); } else { @@ -1661,16 +1712,10 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ // If this is a negative response, send an immediate ACK response. We don't // do this id this response has been passed to us directly from a UACTsx since // PJSIP will have sent and ACK already. - if ((rsp->msg->line.status.code >= 300) && - (!client_rsp)) + if (rsp->msg->line.status.code >= 300) { TRC_DEBUG("Send immediate ACK to negative response"); - pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, - _forks[fork_id].req->msg, - rsp->msg); - ++_pending_sends; - tx_request(ack, fork_id); - _proxy_tsx->schedule_requests(); + tx_negative_ack(rsp, fork_id); } // Notify the sproutlet that an a response has been received. @@ -1678,7 +1723,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ // If this is a 200 OK to a CANCEL, we don't want to do anything with it, so // just drop our reference to it. - if (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id == PJSIP_CANCEL_METHOD) + if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) { pjsip_tx_data_dec_ref(rsp); } @@ -1739,20 +1784,17 @@ void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) { TRC_VERBOSE("%s received CANCEL request", _id.c_str()); - // If this isn't the root sproutlet, send an immediate 200 OK to the CANCEL. - if (_proxy_tsx->_root != this) - { - pjsip_tx_data* ok; - pj_status_t status = PJUtils::create_response(stack_data.endpt, - cancel, - PJSIP_SC_OK, - NULL, - &ok); + TRC_DEBUG("Send immediate 200 OK to CANCEL"); + pjsip_tx_data* ok; + pj_status_t status = PJUtils::create_response(stack_data.endpt, + cancel, + PJSIP_SC_OK, + NULL, + &ok); - if (status == PJ_SUCCESS) - { - tx_response(ok); - } + if (status == PJ_SUCCESS) + { + tx_response(ok); } // Notify the sproutlet that a request has been received. @@ -1949,8 +1991,8 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) return; } - // LCOV_EXCL_START - Sproutlets don't forward 100 Trying responses so we - // should never hit this code. But let's keep it here just in case. + // Sproutlets don't forward 100 Trying responses so we should never hit this + // code. But let's keep it here just in case. if (status_code == 100) { // We will already have sent a locally generated 100 Trying response, so @@ -1960,7 +2002,6 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) pjsip_tx_data_dec_ref(rsp); return; } - // LCOV_EXCL_STOP if ((status_code > 100) && (status_code < 199)) @@ -2054,7 +2095,7 @@ void SproutletWrapper::tx_response(pjsip_tx_data* rsp) // We only treat this as a final response if the status code is >= 200 and // this is not a 200 OK to a CANCEL. if ((rsp->msg->line.status.code >= PJSIP_SC_OK) && - (((pjsip_cseq_hdr*)pjsip_msg_find_hdr(rsp->msg, PJSIP_H_CSEQ, NULL))->method.id != PJSIP_CANCEL_METHOD)) + (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_CANCEL_METHOD)) { _complete = true; cancel_pending_forks(); @@ -2089,10 +2130,22 @@ void SproutletWrapper::tx_cancel(int fork_id) pjsip_tx_data* cancel = PJUtils::create_cancel(stack_data.endpt, _forks[fork_id].req, _forks[fork_id].cancel_reason); + _sproutlet_tsx->obs_tx_request(cancel->msg, fork_id); _proxy_tsx->tx_cancel(this, fork_id, cancel); _forks[fork_id].pending_cancel = false; } +void SproutletWrapper::tx_negative_ack(pjsip_tx_data* rsp, int fork_id) +{ + // Build an ACK request from the original request sent on this fork and the + // negative response received. + pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, + _forks[fork_id].req->msg, + rsp->msg); + _sproutlet_tsx->obs_tx_request(ack->msg, fork_id); + _proxy_tsx->tx_negative_ack(this, fork_id, ack); +} + /// Compare two status codes from the perspective of which is the best to /// return to the originator of a forked transaction. This will only ever /// be called for 3xx/4xx/5xx/6xx response codes. diff --git a/src/ut/mock_sproutlet.h b/src/ut/mock_sproutlet.h index cd73e8111..edf241271 100644 --- a/src/ut/mock_sproutlet.h +++ b/src/ut/mock_sproutlet.h @@ -72,8 +72,10 @@ class MockSproutletTsx : public SproutletTsx MOCK_METHOD1(on_rx_initial_request, void(pjsip_msg*)); MOCK_METHOD1(on_rx_in_dialog_request, void(pjsip_msg*)); + MOCK_METHOD2(obs_rx_request, void(pjsip_msg*)); MOCK_METHOD2(obs_tx_request, void(pjsip_msg*, int)); MOCK_METHOD2(on_rx_response, void(pjsip_msg*, int)); + MOCK_METHOD1(obs_rx_response, void(pjsip_msg*, int)); MOCK_METHOD1(obs_tx_response, void(pjsip_msg*)); MOCK_METHOD2(on_rx_cancel, void(int, pjsip_msg*)); MOCK_METHOD1(on_timer_expiry, void(void*)); diff --git a/src/ut/sproutletproxy_test.cpp b/src/ut/sproutletproxy_test.cpp index 70abbbf7b..6dce5aba7 100644 --- a/src/ut/sproutletproxy_test.cpp +++ b/src/ut/sproutletproxy_test.cpp @@ -154,6 +154,13 @@ class FakeSproutletTsxForwarder : public SproutletTsx { send_response(rsp); } + + void on_rx_trying(pjsip_msg* rsp, int fork_id) + { + // Sproutlets shouldn't call send_response for 100 Trying responses, but + // let's check here that the sproutlet proxy handles it if they do. + send_response(rsp); + } }; class FakeSproutletTsxDownstreamRequest : public SproutletTsx From 7f2dd3a1dd1ea37179f584020d320bfa56c1c82d Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Wed, 1 Mar 2017 09:09:34 +0000 Subject: [PATCH 09/17] Make behaviour of transaction related messages consistent --- include/bgcfsproutlet.h | 4 +- include/icscfsproutlet.h | 8 +- include/scscfsproutlet.h | 4 +- include/sproutlet.h | 20 ++++- include/sproutletproxy.h | 1 + src/bgcfsproutlet.cpp | 10 ++- src/icscfsproutlet.cpp | 114 ++++++++++++++-------------- src/scscfsproutlet.cpp | 63 +++++++++------- src/sproutletproxy.cpp | 158 +++++++++++++++++++++------------------ 9 files changed, 209 insertions(+), 173 deletions(-) diff --git a/include/bgcfsproutlet.h b/include/bgcfsproutlet.h index 937585ac5..bc402aa87 100644 --- a/include/bgcfsproutlet.h +++ b/include/bgcfsproutlet.h @@ -134,9 +134,9 @@ class BGCFSproutletTsx : public SproutletTsx ~BGCFSproutletTsx(); virtual void on_rx_initial_request(pjsip_msg* req) override; - virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void obs_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: diff --git a/include/icscfsproutlet.h b/include/icscfsproutlet.h index 5bace6460..19f81e649 100644 --- a/include/icscfsproutlet.h +++ b/include/icscfsproutlet.h @@ -154,9 +154,9 @@ class ICSCFSproutletTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void obs_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: @@ -200,9 +200,9 @@ class ICSCFSproutletRegTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; - virtual void obs_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; private: diff --git a/include/scscfsproutlet.h b/include/scscfsproutlet.h index f96b52d82..9c6e7f0fd 100644 --- a/include/scscfsproutlet.h +++ b/include/scscfsproutlet.h @@ -245,10 +245,10 @@ class SCSCFSproutletTsx : public SproutletTsx virtual void on_rx_initial_request(pjsip_msg* req) override; virtual void on_rx_in_dialog_request(pjsip_msg* req) override; - virtual void obs_tx_request(pjsip_msg* req, int fork_id) override; + virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) override; virtual void on_rx_response(pjsip_msg* rsp, int fork_id) override; virtual void on_rx_trying(pjsip_msg* rsp, int fork_id) override; - virtual void obs_tx_response(pjsip_msg* rsp) override; + virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) override; virtual void on_rx_cancel(int status_code, pjsip_msg* req) override; virtual void on_timer_expiry(void* context) override; diff --git a/include/sproutlet.h b/include/sproutlet.h index 40e034cec..c31f2eca5 100644 --- a/include/sproutlet.h +++ b/include/sproutlet.h @@ -355,14 +355,20 @@ class SproutletTsx /// Called when a request is received by the sproutlet wrapper. /// /// @param req - The received request. - virtual void obs_rx_request(pjsip_msg* req) {}; + /// @param tsx_mgmt - Whether this is a transaction management message + /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to + /// negative response. + virtual void obs_rx_request(pjsip_msg* req, bool tsx_mgmt) {}; /// Called when a response is received by the sproutlet wrapper. /// /// @param rsp - The received response. /// @param fork_id - The identity of the downstream fork on which the /// response was received. - virtual void obs_rx_response(pjsip_msg* rsp, int fork_id) {}; + /// @param tsx_mgmt - Whether this is a transaction management message + /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to + /// negative response. + virtual void obs_rx_response(pjsip_msg* rsp, int fork_id, bool tsx_mgmt) {}; /// Called when a request has been transmitted on the transaction by the /// wrapper. This is usually because the service has previously called @@ -371,13 +377,19 @@ class SproutletTsx /// @param req - The transmitted request /// @param fork_id - The identity of the downstream fork on which the /// request was sent. - virtual void obs_tx_request(pjsip_msg* req, int fork_id) {} + /// @param tsx_mgmt - Whether this is a transaction management message + /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to + /// negative response. + virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) {} /// Called when a response has been transmitted on the transaction by the /// wrapper. /// /// @param rsp - The transmitted response. - virtual void obs_tx_response(pjsip_msg* rsp) {} + /// @param tsx_mgmt - Whether this is a transaction management message + /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to + /// negative response. + virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) {} /// Called when a timer programmed by the SproutletTsx expires. /// diff --git a/include/sproutletproxy.h b/include/sproutletproxy.h index ff02c119b..39816259e 100644 --- a/include/sproutletproxy.h +++ b/include/sproutletproxy.h @@ -329,6 +329,7 @@ class SproutletWrapper : public SproutletTsxHelper void rx_request(pjsip_tx_data* req); void rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp); void rx_cancel(pjsip_tx_data* cancel); + void rx_negative_ack(pjsip_tx_data* ack); void rx_error(int status_code); void rx_fork_error(pjsip_event_id_e event, int fork_id); void on_timer_pop(TimerID id, void* context); diff --git a/src/bgcfsproutlet.cpp b/src/bgcfsproutlet.cpp index e782c2936..53b6047a3 100644 --- a/src/bgcfsproutlet.cpp +++ b/src/bgcfsproutlet.cpp @@ -253,9 +253,10 @@ void BGCFSproutletTsx::on_rx_initial_request(pjsip_msg* req) } -void BGCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) +void BGCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) { - if (_acr != NULL) + // We don't send ACRs for transaction management messages. + if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted request to the ACR to update the accounting // information. @@ -279,9 +280,10 @@ void BGCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void BGCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) +void BGCFSproutletTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - if (_acr != NULL) + // We don't send ACRs for transaction management messages. + if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted response to the ACR to update the accounting // information. diff --git a/src/icscfsproutlet.cpp b/src/icscfsproutlet.cpp index 32006131c..52d72cffa 100644 --- a/src/icscfsproutlet.cpp +++ b/src/icscfsproutlet.cpp @@ -297,9 +297,10 @@ void ICSCFSproutletRegTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void ICSCFSproutletRegTsx::obs_tx_request(pjsip_msg* req, int fork_id) +void ICSCFSproutletRegTsx::obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) { - if (_acr != NULL) + // We don't send ACRs for transaction management messages. + if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted request to the ACR to update the accounting // information. @@ -393,9 +394,9 @@ void ICSCFSproutletRegTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void ICSCFSproutletRegTsx::obs_tx_response(pjsip_msg* rsp) +void ICSCFSproutletRegTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - if (_acr != NULL) + if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted response to the ACR to update the accounting // information. @@ -710,9 +711,10 @@ void ICSCFSproutletTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void ICSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) +void ICSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) { - if (_acr != NULL) + // We don't send ACRs for transaction management messages. + if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted request to the ACR to update the accounting // information. @@ -799,62 +801,64 @@ void ICSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) } -void ICSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) +void ICSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - if (_acr != NULL) - { - // Pass the transmitted response to the ACR to update the accounting - // information. - _acr->tx_response(rsp); - } - - pjsip_status_code rsp_status = (pjsip_status_code)rsp->line.status.code; - - // Check if this is a terminating INVITE. If it is then check whether we need - // to update our session establishment stats. We consider the session to be - // set up as soon as we receive a final response OR a 180 Ringing (per TS - // 32.409). We do not consider responses to CANCEL requests as final - // responses. - if (!_originating && - (_req_type == PJSIP_INVITE_METHOD) && - !_session_set_up && - ((rsp_status == PJSIP_SC_RINGING) || - !PJSIP_IS_STATUS_IN_CLASS(rsp_status, 100)) && - (PJSIP_MSG_CSEQ_HDR(rsp)->method.id != PJSIP_CANCEL_METHOD)) + // We don't do this processing for transaction management messages. + if (!tsx_mgmt) { - // Session is now set up. - _session_set_up = true; - _icscf->_session_establishment_tbl->increment_attempts(); - _icscf->_session_establishment_network_tbl->increment_attempts(); - - if ((rsp_status == PJSIP_SC_RINGING) || - PJSIP_IS_STATUS_IN_CLASS(rsp_status, 200)) - { - // Session has been set up successfully. - TRC_DEBUG("Session successful"); - _icscf->_session_establishment_tbl->increment_successes(); - _icscf->_session_establishment_network_tbl->increment_successes(); - } - else if ((rsp_status == PJSIP_SC_BUSY_HERE) || - (rsp_status == PJSIP_SC_BUSY_EVERYWHERE) || - (rsp_status == PJSIP_SC_NOT_FOUND) || - (rsp_status == PJSIP_SC_ADDRESS_INCOMPLETE)) + if (_acr != NULL) { - // Session failed, but should be counted as successful from a network - // perspective. - TRC_DEBUG("Session failed but network successful"); - _icscf->_session_establishment_tbl->increment_failures(); - _icscf->_session_establishment_network_tbl->increment_successes(); + // Pass the transmitted response to the ACR to update the accounting + // information. + _acr->tx_response(rsp); } - else + + pjsip_status_code rsp_status = (pjsip_status_code)rsp->line.status.code; + + // Check if this is a terminating INVITE. If it is then check whether we need + // to update our session establishment stats. We consider the session to be + // set up as soon as we receive a final response OR a 180 Ringing (per TS + // 32.409). We do not consider responses to CANCEL requests as final + // responses. + if (!_originating && + (_req_type == PJSIP_INVITE_METHOD) && + !_session_set_up && + ((rsp_status == PJSIP_SC_RINGING) || + !PJSIP_IS_STATUS_IN_CLASS(rsp_status, 100))) { - // Session establishment failed. - TRC_DEBUG("Session failed"); - _icscf->_session_establishment_tbl->increment_failures(); - _icscf->_session_establishment_network_tbl->increment_failures(); + // Session is now set up. + _session_set_up = true; + _icscf->_session_establishment_tbl->increment_attempts(); + _icscf->_session_establishment_network_tbl->increment_attempts(); + + if ((rsp_status == PJSIP_SC_RINGING) || + PJSIP_IS_STATUS_IN_CLASS(rsp_status, 200)) + { + // Session has been set up successfully. + TRC_DEBUG("Session successful"); + _icscf->_session_establishment_tbl->increment_successes(); + _icscf->_session_establishment_network_tbl->increment_successes(); + } + else if ((rsp_status == PJSIP_SC_BUSY_HERE) || + (rsp_status == PJSIP_SC_BUSY_EVERYWHERE) || + (rsp_status == PJSIP_SC_NOT_FOUND) || + (rsp_status == PJSIP_SC_ADDRESS_INCOMPLETE)) + { + // Session failed, but should be counted as successful from a network + // perspective. + TRC_DEBUG("Session failed but network successful"); + _icscf->_session_establishment_tbl->increment_failures(); + _icscf->_session_establishment_network_tbl->increment_successes(); + } + else + { + // Session establishment failed. + TRC_DEBUG("Session failed"); + _icscf->_session_establishment_tbl->increment_failures(); + _icscf->_session_establishment_network_tbl->increment_failures(); + } } } - } diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index 15f7875a2..f2a23570a 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -581,16 +581,20 @@ void SCSCFSproutletTsx::on_rx_in_dialog_request(pjsip_msg* req) } -void SCSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id) +void SCSCFSproutletTsx::obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) { - ACR* acr = get_acr(); - if (acr) + // We don't send ACRs for transaction management messages. + if (!tsx_mgmt) { - // Pass the transmitted request to the ACR to update the accounting - // information. - acr->lock(); - acr->tx_request(req); - acr->unlock(); + ACR* acr = get_acr(); + if (acr) + { + // Pass the transmitted request to the ACR to update the accounting + // information. + acr->lock(); + acr->tx_request(req); + acr->unlock(); + } } } @@ -717,29 +721,32 @@ void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp, int fork_id) } -void SCSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp) +void SCSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - ACR* acr = get_acr(); - if (acr != NULL) + // We don't do this processing for transaction management messages. + if (!tsx_mgmt) { - // Pass the transmitted response to the ACR to update the accounting - // information. - acr->lock(); - acr->tx_response(rsp); - acr->unlock(); - } + ACR* acr = get_acr(); + if (acr != NULL) + { + // Pass the transmitted response to the ACR to update the accounting + // information. + acr->lock(); + acr->tx_response(rsp); + acr->unlock(); + } - // If this is a transaction where we are supposed to be tracking session - // setup stats then check to see if it is now set up. We consider it to be - // setup when we receive either a 180 Ringing or 2xx (per TS 32.409). - pjsip_status_code st_code = (pjsip_status_code)rsp->line.status.code; - if (_record_session_setup_time && - ((st_code == PJSIP_SC_RINGING) || - ((PJSIP_IS_STATUS_IN_CLASS(st_code, 200)) && - (PJSIP_MSG_CSEQ_HDR(rsp)->method.id != PJSIP_CANCEL_METHOD)))) - { - _scscf->track_session_setup_time(_tsx_start_time_usec, _video_call); - _record_session_setup_time = false; + // If this is a transaction where we are supposed to be tracking session + // setup stats then check to see if it is now set up. We consider it to be + // setup when we receive either a 180 Ringing or 2xx (per TS 32.409). + pjsip_status_code st_code = (pjsip_status_code)rsp->line.status.code; + if (_record_session_setup_time && + ((st_code == PJSIP_SC_RINGING) || + (PJSIP_IS_STATUS_IN_CLASS(st_code, 200)))) + { + _scscf->track_session_setup_time(_tsx_start_time_usec, _video_call); + _record_session_setup_time = false; + } } } diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 03942b8eb..6fc20fd20 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -869,22 +869,6 @@ void SproutletProxy::UASTsx::schedule_requests() _umap[(void*)downstream] = req.upstream; } - if (req.req->msg->line.req.method.id == PJSIP_INVITE_METHOD) - { - // Send an immediate 100 Trying response to the upstream - // Sproutlet. - pjsip_tx_data* trying; - pj_status_t status = PJUtils::create_response(stack_data.endpt, - req.req, - PJSIP_SC_TRYING, - NULL, - &trying); - if (status == PJ_SUCCESS) - { - req.upstream.first->rx_response(trying, req.upstream.second, false); - } - } - // Pass the request to the downstream sproutlet. downstream->rx_request(req.req); } @@ -991,9 +975,10 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, { if (downstream == _root) { - // If this is the root sproutlet in the tree, drop any 200 OK to CANCEL - // messages that the sproutlets generated. - if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) + // If this is the root sproutlet in the tree, drop any 100 Trying or 200 OK + // to CANCEL responses that the sproutlets generated. + if ((PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) || + (rsp->msg->line.status.code == PJSIP_SC_TRYING)) { pjsip_tx_data_dec_ref(rsp); return; @@ -1111,7 +1096,7 @@ void SproutletProxy::UASTsx::tx_negative_ack(SproutletWrapper* upstream, // Pass the ACK request to the downstream Sproutlet. SproutletWrapper* downstream = i->second; TRC_DEBUG("Route negative ACK to %s", downstream->service_name().c_str()); - downstream->rx_request(ack); + downstream->rx_negative_ack(ack); // This is an ACK to a final response so disconnect the sproutlets. _dmap_sproutlet.erase(std::make_pair(upstream, fork_id)); @@ -1634,6 +1619,22 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) event.add_var_param(_service_name); SAS::report_event(event); + if (req->msg->line.req.method.id == PJSIP_INVITE_METHOD) + { + // Send an immediate 100 Trying response to the upstream + // Sproutlet. + pjsip_tx_data* trying; + pj_status_t status = PJUtils::create_response(stack_data.endpt, + req, + PJSIP_SC_TRYING, + NULL, + &trying); + if (status == PJ_SUCCESS) + { + tx_response(trying); + } + } + // Log the request at VERBOSE level before we send it out to aid in // tracking its path through the sproutlets. if (Log::enabled(Log::VERBOSE_LEVEL)) @@ -1641,57 +1642,46 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) log_inter_sproutlet(req, true); } - // Notify the sproutlet that an a request has been received. - _sproutlet_tsx->obs_rx_request(req->msg); + // Notify the sproutlet that a request has been received. + _sproutlet_tsx->obs_rx_request(req->msg, false); + + // Keep an immutable reference to the request. + _req = req; - // If this is an ACK request and we have already decided to absorb ACK - // requests in the sproutlets, drop our reference to this request. - if ((req->msg->line.req.method.id == PJSIP_ACK_METHOD) && - (_proxy_tsx->_absorb_acks)) + // Clone the request to get a mutable copy to pass to the Sproutlet. + pjsip_msg* clone = original_request(); + if (clone == NULL) { - TRC_DEBUG("Absorb ACK to negative response"); - pjsip_tx_data_dec_ref(req); + // @TODO } - else - { - // Keep an immutable reference to the request. - _req = req; - - // Clone the request to get a mutable copy to pass to the Sproutlet. - pjsip_msg* clone = original_request(); - if (clone == NULL) - { - // @TODO - } - - // Decrement Max-Forwards if present. - pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) - pjsip_msg_find_hdr(clone, PJSIP_H_MAX_FORWARDS, NULL); - if (mf_hdr != NULL) - { - --mf_hdr->ivalue; - } - if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) - { - TRC_VERBOSE("%s pass initial request %s to Sproutlet", - _id.c_str(), msg_info(clone)); - _sproutlet_tsx->on_rx_initial_request(clone); - } - else - { - TRC_VERBOSE("%s pass in dialog request %s to Sproutlet", - _id.c_str(), msg_info(clone)); - _sproutlet_tsx->on_rx_in_dialog_request(clone); - } + // Decrement Max-Forwards if present. + pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) + pjsip_msg_find_hdr(clone, PJSIP_H_MAX_FORWARDS, NULL); + if (mf_hdr != NULL) + { + --mf_hdr->ivalue; + } - // We consider an ACK transaction to be complete immediately after the - // sproutlet's actions have been processed, regardless of whether the - // sproutlet forwarded the ACK (some sproutlets are unable to in certain - // situations). - bool complete_after_actions = (req->msg->line.req.method.id == PJSIP_ACK_METHOD); - process_actions(complete_after_actions); + if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) + { + TRC_VERBOSE("%s pass initial request %s to Sproutlet", + _id.c_str(), msg_info(clone)); + _sproutlet_tsx->on_rx_initial_request(clone); + } + else + { + TRC_VERBOSE("%s pass in dialog request %s to Sproutlet", + _id.c_str(), msg_info(clone)); + _sproutlet_tsx->on_rx_in_dialog_request(clone); } + + // We consider an ACK transaction to be complete immediately after the + // sproutlet's actions have been processed, regardless of whether the + // sproutlet forwarded the ACK (some sproutlets are unable to in certain + // situations). + bool complete_after_actions = (req->msg->line.req.method.id == PJSIP_ACK_METHOD); + process_actions(complete_after_actions); } void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp) @@ -1718,13 +1708,12 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ tx_negative_ack(rsp, fork_id); } - // Notify the sproutlet that an a response has been received. - _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id); - // If this is a 200 OK to a CANCEL, we don't want to do anything with it, so - // just drop our reference to it. + // just notify the sproutlet that it has been received and then drop our + // reference to it. if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) { + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); pjsip_tx_data_dec_ref(rsp); } else @@ -1770,10 +1759,12 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ if (status_code == PJSIP_SC_TRYING) { + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); } else { + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, false); _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); } process_actions(false); @@ -1798,7 +1789,7 @@ void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) } // Notify the sproutlet that a request has been received. - _sproutlet_tsx->obs_rx_request(cancel->msg); + _sproutlet_tsx->obs_rx_request(cancel->msg, true); _sproutlet_tsx->on_rx_cancel(PJSIP_SC_REQUEST_TERMINATED, cancel->msg); @@ -1807,6 +1798,17 @@ void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) process_actions(false); } +void SproutletWrapper::rx_negative_ack(pjsip_tx_data* ack) +{ + TRC_VERBOSE("%s received negative ACK request", _id.c_str()); + + // Notify the sproutlet that an a negative ACK has been received and then + // drop our reference to this request. + _sproutlet_tsx->obs_rx_request(ack->msg, true); + + pjsip_tx_data_dec_ref(ack); +} + void SproutletWrapper::rx_error(int status_code) { TRC_VERBOSE("%s received error %d", _id.c_str(), status_code); @@ -2083,7 +2085,7 @@ void SproutletWrapper::tx_request(pjsip_tx_data* req, int fork_id) } // Notify the sproutlet that the request is being sent downstream. - _sproutlet_tsx->obs_tx_request(req->msg, fork_id); + _sproutlet_tsx->obs_tx_request(req->msg, fork_id, false); // Forward the request downstream. deregister_tdata(req); @@ -2116,7 +2118,15 @@ void SproutletWrapper::tx_response(pjsip_tx_data* rsp) } // Notify the sproutlet that the response is being sent upstream. - _sproutlet_tsx->obs_tx_response(rsp->msg); + if ((PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) || + (rsp->msg->line.status.code == PJSIP_SC_TRYING)) + { + _sproutlet_tsx->obs_tx_response(rsp->msg, true); + } + else + { + _sproutlet_tsx->obs_tx_response(rsp->msg, false); + } // Forward the response upstream. deregister_tdata(rsp); @@ -2130,7 +2140,7 @@ void SproutletWrapper::tx_cancel(int fork_id) pjsip_tx_data* cancel = PJUtils::create_cancel(stack_data.endpt, _forks[fork_id].req, _forks[fork_id].cancel_reason); - _sproutlet_tsx->obs_tx_request(cancel->msg, fork_id); + _sproutlet_tsx->obs_tx_request(cancel->msg, fork_id, true); _proxy_tsx->tx_cancel(this, fork_id, cancel); _forks[fork_id].pending_cancel = false; } @@ -2142,7 +2152,7 @@ void SproutletWrapper::tx_negative_ack(pjsip_tx_data* rsp, int fork_id) pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, _forks[fork_id].req->msg, rsp->msg); - _sproutlet_tsx->obs_tx_request(ack->msg, fork_id); + _sproutlet_tsx->obs_tx_request(ack->msg, fork_id, true); _proxy_tsx->tx_negative_ack(this, fork_id, ack); } From 48f465a8bb7ca0a6a68f87e285de0c579f23bed4 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Thu, 2 Mar 2017 09:46:48 +0000 Subject: [PATCH 10/17] Fixes from live test --- include/sproutletproxy.h | 2 +- src/sproutletproxy.cpp | 58 ++++++++++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/include/sproutletproxy.h b/include/sproutletproxy.h index 39816259e..7dab9676c 100644 --- a/include/sproutletproxy.h +++ b/include/sproutletproxy.h @@ -327,7 +327,7 @@ class SproutletWrapper : public SproutletTsxHelper private: void rx_request(pjsip_tx_data* req); - void rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp); + void rx_response(pjsip_tx_data* rsp, int fork_id); void rx_cancel(pjsip_tx_data* cancel); void rx_negative_ack(pjsip_tx_data* ack); void rx_error(int status_code); diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 6fc20fd20..7a4ced417 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -671,7 +671,7 @@ void SproutletProxy::UASTsx::on_new_client_response(UACTsx* uac_tsx, _umap.erase(i); } - upstream_sproutlet->rx_response(rsp, upstream_fork, true); + upstream_sproutlet->rx_response(rsp, upstream_fork); // Schedule any requests generated by the Sproutlet. schedule_requests(); @@ -835,7 +835,7 @@ void SproutletProxy::UASTsx::schedule_requests() if (status == PJ_SUCCESS) { // Pass the response back to the Sproutlet. - req.upstream.first->rx_response(rsp, req.upstream.second, false); + req.upstream.first->rx_response(rsp, req.upstream.second); } } else @@ -984,6 +984,18 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, return; } + // If this is a negative response, send an ACK upstream to the root + // sproutlet. + if (rsp->msg->line.status.code >= 300) + { + // Build an ACK request from the original request sent on this fork and the + // negative response received. + pjsip_tx_data* ack = PJUtils::create_ack(stack_data.endpt, + downstream->_req->msg, + rsp->msg); + downstream->rx_negative_ack(ack); + } + // This is the root sproutlet in the tree, so send the response on the // UAS transaction. if (_tsx != NULL) @@ -1032,7 +1044,7 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, _dmap_sproutlet.erase(i->second); _umap.erase(i); } - upstream->rx_response(rsp, fork_id, false); + upstream->rx_response(rsp, fork_id); } else { @@ -1067,6 +1079,19 @@ void SproutletProxy::UASTsx::tx_cancel(SproutletWrapper* upstream, } else { + TRC_DEBUG("Send immediate 200 OK to CANCEL"); + pjsip_tx_data* ok; + pj_status_t status = PJUtils::create_response(stack_data.endpt, + cancel, + PJSIP_SC_OK, + NULL, + &ok); + + if (status == PJ_SUCCESS) + { + upstream->rx_response(ok, fork_id); + } + DMap::iterator j = _dmap_uac.find(std::make_pair(upstream, fork_id)); if (j != _dmap_uac.end()) { @@ -1619,6 +1644,16 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) event.add_var_param(_service_name); SAS::report_event(event); + // Log the request at VERBOSE level before we send it out to aid in + // tracking its path through the sproutlets. + if (Log::enabled(Log::VERBOSE_LEVEL)) + { + log_inter_sproutlet(req, true); + } + + // Notify the sproutlet that a request has been received. + _sproutlet_tsx->obs_rx_request(req->msg, false); + if (req->msg->line.req.method.id == PJSIP_INVITE_METHOD) { // Send an immediate 100 Trying response to the upstream @@ -1631,20 +1666,11 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) &trying); if (status == PJ_SUCCESS) { + log_inter_sproutlet(trying, false); tx_response(trying); } } - // Log the request at VERBOSE level before we send it out to aid in - // tracking its path through the sproutlets. - if (Log::enabled(Log::VERBOSE_LEVEL)) - { - log_inter_sproutlet(req, true); - } - - // Notify the sproutlet that a request has been received. - _sproutlet_tsx->obs_rx_request(req->msg, false); - // Keep an immutable reference to the request. _req = req; @@ -1684,7 +1710,7 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) process_actions(complete_after_actions); } -void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_rsp) +void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) { // SAS log the start of processing by this sproutlet SAS::Event event(trail(), SASEvent::BEGIN_SPROUTLET_RSP, 0); @@ -1699,9 +1725,7 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id, bool client_ log_inter_sproutlet(rsp, false); } - // If this is a negative response, send an immediate ACK response. We don't - // do this id this response has been passed to us directly from a UACTsx since - // PJSIP will have sent and ACK already. + // If this is a negative response, send an immediate ACK response. if (rsp->msg->line.status.code >= 300) { TRC_DEBUG("Send immediate ACK to negative response"); From 5f101147a212c08c53a53d7ee9d6b373bd0029a7 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Thu, 2 Mar 2017 10:17:05 +0000 Subject: [PATCH 11/17] Update comments --- src/icscfsproutlet.cpp | 3 ++- src/scscfsproutlet.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/icscfsproutlet.cpp b/src/icscfsproutlet.cpp index 52d72cffa..4dc379854 100644 --- a/src/icscfsproutlet.cpp +++ b/src/icscfsproutlet.cpp @@ -396,6 +396,7 @@ void ICSCFSproutletRegTsx::on_rx_response(pjsip_msg* rsp, int fork_id) void ICSCFSproutletRegTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { + // We don't send ACRs for transaction management messages. if ((!tsx_mgmt) && (_acr != NULL)) { // Pass the transmitted response to the ACR to update the accounting @@ -803,7 +804,7 @@ void ICSCFSproutletTsx::on_rx_response(pjsip_msg* rsp, int fork_id) void ICSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - // We don't do this processing for transaction management messages. + // We don't send ACRs or update stats for transaction management messages. if (!tsx_mgmt) { if (_acr != NULL) diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index 9ef47e7ad..4f17d690c 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -723,7 +723,7 @@ void SCSCFSproutletTsx::on_rx_trying(pjsip_msg* rsp, int fork_id) void SCSCFSproutletTsx::obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) { - // We don't do this processing for transaction management messages. + // We don't send ACRs or update stats for transaction management messages. if (!tsx_mgmt) { ACR* acr = get_acr(); From 61d79c3c921d04d14f22e3b693ac7152ccf1ec6e Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Thu, 2 Mar 2017 15:47:15 +0000 Subject: [PATCH 12/17] Store off request earlier --- src/sproutletproxy.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 7a4ced417..a331ade9a 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1651,6 +1651,9 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) log_inter_sproutlet(req, true); } + // Keep an immutable reference to the request. + _req = req; + // Notify the sproutlet that a request has been received. _sproutlet_tsx->obs_rx_request(req->msg, false); @@ -1671,9 +1674,6 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) } } - // Keep an immutable reference to the request. - _req = req; - // Clone the request to get a mutable copy to pass to the Sproutlet. pjsip_msg* clone = original_request(); if (clone == NULL) From 177776f8a8422865aac382d3a431755aa5361b61 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Thu, 2 Mar 2017 16:28:50 +0000 Subject: [PATCH 13/17] Reordering --- src/sproutletproxy.cpp | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index a331ade9a..3240633c2 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1725,8 +1725,19 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) log_inter_sproutlet(rsp, false); } + + int status_code = rsp->msg->line.status.code; + if (status_code == PJSIP_SC_TRYING) + { + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); + } + else if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_CANCEL_METHOD) + { + _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, false); + } + // If this is a negative response, send an immediate ACK response. - if (rsp->msg->line.status.code >= 300) + if (status_code >= 300) { TRC_DEBUG("Send immediate ACK to negative response"); tx_negative_ack(rsp, fork_id); @@ -1743,7 +1754,6 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) else { register_tdata(rsp); - int status_code = rsp->msg->line.status.code; if ((PJSIP_IS_STATUS_IN_CLASS(status_code, 100)) && (_forks[fork_id].state.tsx_state == PJSIP_TSX_STATE_CALLING)) { @@ -1783,12 +1793,10 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) if (status_code == PJSIP_SC_TRYING) { - _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); } else { - _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, false); _sproutlet_tsx->on_rx_response(rsp->msg, fork_id); } process_actions(false); @@ -1799,6 +1807,9 @@ void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) { TRC_VERBOSE("%s received CANCEL request", _id.c_str()); + // Notify the sproutlet that a request has been received. + _sproutlet_tsx->obs_rx_request(cancel->msg, true); + TRC_DEBUG("Send immediate 200 OK to CANCEL"); pjsip_tx_data* ok; pj_status_t status = PJUtils::create_response(stack_data.endpt, @@ -1812,9 +1823,6 @@ void SproutletWrapper::rx_cancel(pjsip_tx_data* cancel) tx_response(ok); } - // Notify the sproutlet that a request has been received. - _sproutlet_tsx->obs_rx_request(cancel->msg, true); - _sproutlet_tsx->on_rx_cancel(PJSIP_SC_REQUEST_TERMINATED, cancel->msg); pjsip_tx_data_dec_ref(cancel); From 50a68506fe06acdb9479250c96f352c737d48a6c Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Fri, 3 Mar 2017 16:01:58 +0000 Subject: [PATCH 14/17] Markups --- include/pjutils.h | 8 +++ include/sproutlet.h | 40 +++++++------ include/sproutletproxy.h | 7 +-- src/mangelwurzel/ut/mangelwurzel_test.cpp | 22 +++---- src/scscfsproutlet.cpp | 1 - src/sproutletappserver.cpp | 2 +- src/sproutletproxy.cpp | 73 ++++++++++------------- src/ut/mock_sproutlet.h | 8 +-- src/ut/mocktsxhelper.h | 2 +- 9 files changed, 78 insertions(+), 85 deletions(-) diff --git a/include/pjutils.h b/include/pjutils.h index b1fbd3655..77e7a53ce 100644 --- a/include/pjutils.h +++ b/include/pjutils.h @@ -179,6 +179,14 @@ pjsip_tx_data* create_cancel(pjsip_endpoint* endpt, pjsip_tx_data* tdata, int reason_code); +/// Creates an ACK to a negative response. This method cannot be used to +/// generate ACKs for ACK transactions. +/// +/// @return - The negative ACK created +/// +/// @param endpt - The PJSIP endpoint. +/// @param req - The original request that the response was sent to. +/// @param rsp - The response to the original request. pjsip_tx_data* create_ack(pjsip_endpoint* endpt, pjsip_msg* req, pjsip_msg* rsp); diff --git a/include/sproutlet.h b/include/sproutlet.h index c31f2eca5..a746420b7 100644 --- a/include/sproutlet.h +++ b/include/sproutlet.h @@ -91,7 +91,7 @@ class SproutletTsxHelper /// /// @returns - A clone of the original request message. /// - virtual pjsip_msg* original_request() = 0; + virtual pjsip_msg* get_request_for_sproutlet_tsx() = 0; /// Returns the top Route header from the original incoming request. This /// can be inpsected by the Sproutlet, but should not be modified. Note that @@ -352,43 +352,45 @@ class SproutletTsx /// was triggered by an error or timeout. virtual void on_rx_cancel(int status_code, pjsip_msg* cancel_req) {} - /// Called when a request is received by the sproutlet wrapper. + /// The next four methods make up the observation API. The purpose of this API + /// is to view transaction management messages that are not seen by + /// on_rx_request and on_rx_response. This includes CANCEL, 200 OK to CANCEL, + /// 100 Trying and ACK to negative response. This API also sees all the + /// messages that are passed to on_rx_request and on_rx_response. + + /// Called when a request is received by this sproutlet Tsx. /// /// @param req - The received request. - /// @param tsx_mgmt - Whether this is a transaction management message - /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to - /// negative response. + /// @param tsx_mgmt - True when the received request will not be passed + /// to on_rx_request. virtual void obs_rx_request(pjsip_msg* req, bool tsx_mgmt) {}; - /// Called when a response is received by the sproutlet wrapper. + /// Called when a response is received by this sproutlet Tsx. /// /// @param rsp - The received response. /// @param fork_id - The identity of the downstream fork on which the /// response was received. - /// @param tsx_mgmt - Whether this is a transaction management message - /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to - /// negative response. + /// @param tsx_mgmt - True when the received respone will not be passed + /// to on_rx_response. virtual void obs_rx_response(pjsip_msg* rsp, int fork_id, bool tsx_mgmt) {}; - /// Called when a request has been transmitted on the transaction by the - /// wrapper. This is usually because the service has previously called + /// Called when a request has been transmitted on the transaction by this + /// sproutlet Tsx. This is usually because the service has previously called /// forward_request() with the request message. /// /// @param req - The transmitted request /// @param fork_id - The identity of the downstream fork on which the /// request was sent. - /// @param tsx_mgmt - Whether this is a transaction management message - /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to - /// negative response. + /// @param tsx_mgmt - True when the transmitted request was not generated + /// by the sproutlet Tsx calling send_request(). virtual void obs_tx_request(pjsip_msg* req, int fork_id, bool tsx_mgmt) {} /// Called when a response has been transmitted on the transaction by the - /// wrapper. + /// sproutlet Tsx. /// /// @param rsp - The transmitted response. - /// @param tsx_mgmt - Whether this is a transaction management message - /// e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to - /// negative response. + /// @param tsx_mgmt - True when the transmitted response was not generated + /// by the sproutlet Tsx calling send_response(). virtual void obs_tx_response(pjsip_msg* rsp, bool tsx_mgmt) {} /// Called when a timer programmed by the SproutletTsx expires. @@ -405,7 +407,7 @@ class SproutletTsx /// @returns - A clone of the original request message. /// pjsip_msg* original_request() - {return _helper->original_request();} + {return _helper->get_request_for_sproutlet_tsx();} /// Returns a URI that could be used to route back to the current Sproutlet. /// This URI may contain pre-loaded parameters that should not be modified diff --git a/include/sproutletproxy.h b/include/sproutletproxy.h index 7dab9676c..aa83fd518 100644 --- a/include/sproutletproxy.h +++ b/include/sproutletproxy.h @@ -256,11 +256,6 @@ class SproutletProxy : public BasicProxy /// The UASTsx will persist while there are pending timers. std::set _pending_timers; - /// If flag is set, any ACKs generated in the sproutlet wrapper will be - /// absorbed. This is so that we do not send ACKs to negative responses off - /// the box. - bool _absorb_acks; - friend class SproutletWrapper; }; @@ -298,7 +293,7 @@ class SproutletWrapper : public SproutletTsxHelper /// functions from SproutletTsxHelper. See there for function comments for /// the following. void add_to_dialog(const std::string& dialog_id=""); - pjsip_msg* original_request(); + pjsip_msg* get_request_for_sproutlet_tsx(); const char* msg_info(pjsip_msg*); const pjsip_route_hdr* route_hdr() const; const std::string& dialog_id() const; diff --git a/src/mangelwurzel/ut/mangelwurzel_test.cpp b/src/mangelwurzel/ut/mangelwurzel_test.cpp index 064f544d6..2f226b9e9 100644 --- a/src/mangelwurzel/ut/mangelwurzel_test.cpp +++ b/src/mangelwurzel/ut/mangelwurzel_test.cpp @@ -179,7 +179,7 @@ MATCHER_P(ReqUriEquals, req_uri, "") TEST_F(MangelwurzelTest, Rot13) { MangelwurzelTsx::Config config; - EXPECT_CALL(*_helper, original_request()); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()); EXPECT_CALL(*_helper, free_msg(_)); MangelwurzelTsx mangelwurzel_tsx(_helper, config); @@ -200,7 +200,7 @@ TEST_F(MangelwurzelTest, Rot13) TEST_F(MangelwurzelTest, Reverse) { MangelwurzelTsx::Config config; - EXPECT_CALL(*_helper, original_request()); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()); EXPECT_CALL(*_helper, free_msg(_)); MangelwurzelTsx mangelwurzel_tsx(_helper, config); @@ -224,7 +224,7 @@ TEST_F(MangelwurzelTest, CreateDefaults) Mangelwurzel mangelwurzel("mangelwurzel", 5058, "sip:mangelwurzel.homedomain:5058;transport=tcp"); Message msg; pjsip_msg* req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(req)); EXPECT_CALL(*_helper, free_msg(req)); pjsip_route_hdr* hdr = pjsip_rr_hdr_create(stack_data.pool); @@ -255,7 +255,7 @@ TEST_F(MangelwurzelTest, CreateFullConfig) Mangelwurzel mangelwurzel("mangelwurzel", 5058, "sip:mangelwurzel.homedomain:5058;transport=tcp"); Message msg; pjsip_msg* req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(req)); EXPECT_CALL(*_helper, free_msg(req)); pjsip_route_hdr* hdr = pjsip_rr_hdr_create(stack_data.pool); @@ -290,7 +290,7 @@ TEST_F(MangelwurzelTest, CreateRot13) Mangelwurzel mangelwurzel("mangelwurzel", 5058, "sip:mangelwurzel.homedomain:5058;transport=tcp"); Message msg; pjsip_msg* req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(req)); EXPECT_CALL(*_helper, free_msg(req)); pjsip_route_hdr* hdr = pjsip_rr_hdr_create(stack_data.pool); @@ -317,7 +317,7 @@ TEST_F(MangelwurzelTest, CreateInvalidMangalgorithm) Mangelwurzel mangelwurzel("mangelwurzel", 5058, "sip:mangelwurzel.homedomain:5058;transport=tcp"); Message msg; pjsip_msg* req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(req)); EXPECT_CALL(*_helper, free_msg(req)); pjsip_route_hdr* hdr = pjsip_rr_hdr_create(stack_data.pool); @@ -347,7 +347,7 @@ TEST_F(MangelwurzelTest, CreateNoRouteHdr) Message msg; msg._requri = "sip:mangelwurzel.homedomain;mangalgorithm=reverse"; pjsip_msg* req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(req)); EXPECT_CALL(*_helper, free_msg(req)); EXPECT_CALL(*_helper, route_hdr()).WillOnce(ReturnNull()); @@ -372,7 +372,7 @@ TEST_F(MangelwurzelTest, InitialReq) // Save off the original request. We expect mangelwurzel to request it later. pjsip_msg* original_req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(original_req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(original_req)); EXPECT_CALL(*_helper, free_msg(original_req)); // Set up the mangelwurzel transaction's config. Turn everything on. @@ -427,7 +427,7 @@ TEST_F(MangelwurzelTest, Response) // Save off the original request. We expect mangelwurzel to request it later. pjsip_msg* original_req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(original_req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(original_req)); EXPECT_CALL(*_helper, free_msg(original_req)); // Set up the mangelwurzel transaction's config. Turn everything on. @@ -475,7 +475,7 @@ TEST_F(MangelwurzelTest, InDialogReq) // Save off the original request. We expect mangelwurzel to request it later. pjsip_msg* original_req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(original_req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(original_req)); EXPECT_CALL(*_helper, free_msg(original_req)); // Set up the mangelwurzel transaction's config. This is different to the @@ -523,7 +523,7 @@ TEST_F(MangelwurzelTest, REGISTER) // Save off the original request. We expect mangelwurzel to request it later. pjsip_msg* original_req = parse_msg(msg.get_request()); - EXPECT_CALL(*_helper, original_request()).WillOnce(Return(original_req)); + EXPECT_CALL(*_helper, get_request_for_sproutlet_tsx()).WillOnce(Return(original_req)); EXPECT_CALL(*_helper, free_msg(original_req)); // Set up the mangelwurzel transaction's config. This is different to the diff --git a/src/scscfsproutlet.cpp b/src/scscfsproutlet.cpp index 7f8464123..dad93e752 100644 --- a/src/scscfsproutlet.cpp +++ b/src/scscfsproutlet.cpp @@ -2220,7 +2220,6 @@ std::string SCSCFSproutletTsx::fork_failure_reason_as_string(int fork_id, int si return reason; } -<<<<<<< HEAD void SCSCFSproutletTsx::acr_handle_response(pjsip_msg* rsp) { // Pass the received response to the ACR. diff --git a/src/sproutletappserver.cpp b/src/sproutletappserver.cpp index 40321a18d..c7e31f708 100644 --- a/src/sproutletappserver.cpp +++ b/src/sproutletappserver.cpp @@ -107,7 +107,7 @@ void SproutletAppServerTsxHelper::store_dialog_id(pjsip_msg* req) /// pjsip_msg* SproutletAppServerTsxHelper::original_request() { - return _helper->original_request(); + return _helper->get_request_for_sproutlet_tsx(); } /// Returns the top Route header from the original incoming request. This diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 3240633c2..0132b1c79 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -545,20 +545,6 @@ pj_status_t SproutletProxy::UASTsx::init(pjsip_rx_data* rdata) // Do the BasicProxy initialization first. pj_status_t status = BasicProxy::UASTsx::init(rdata); - if (rdata->msg_info.msg->line.req.method.id != PJSIP_ACK_METHOD) - { - // Sproutlets may generate local ACKs to negative responses, we should not - // send these off the box, so set a flag to absorb ACKs that are received - // by the sproutlet wrappers. - _absorb_acks = true; - } - else - { - // For ACK transactions, we should not absorb ACKS that are generated by - // the sproutlets. - _absorb_acks = false; - } - if (status == PJ_SUCCESS) { // Locate the target Sproutlet for the request, and create the helper and @@ -976,7 +962,8 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, if (downstream == _root) { // If this is the root sproutlet in the tree, drop any 100 Trying or 200 OK - // to CANCEL responses that the sproutlets generated. + // to CANCEL responses that the sproutlets generated. These responses have + // already been sent by the basic proxy. if ((PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) || (rsp->msg->line.status.code == PJSIP_SC_TRYING)) { @@ -984,9 +971,10 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, return; } - // If this is a negative response, send an ACK upstream to the root - // sproutlet. - if (rsp->msg->line.status.code >= 300) + // If this is a negative response to an INVITE, send an ACK downstream to + // the root sproutlet. + if ((PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_INVITE_METHOD) && + (rsp->msg->line.status.code >= 300)) { // Build an ACK request from the original request sent on this fork and the // negative response received. @@ -1033,14 +1021,17 @@ void SproutletProxy::UASTsx::tx_response(SproutletWrapper* downstream, SproutletWrapper* upstream = i->second.first; int fork_id = i->second.second; - // We do not treat 200 OK to CANCEL as a final response. + // We break the linkage between the sproutlets if: + // - We receive a 2xx response for an INVITE. For 3xx-6xx responses + // we will break the linkage after sending an ACK to the negative + // response. + // - We receive a final response for other transaction types (excluding + // CANCEL). if ((rsp->msg->line.status.code >= PJSIP_SC_OK) && - (rsp->msg->line.status.code < 300) && + ((PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_INVITE_METHOD) || + (rsp->msg->line.status.code < 300)) && (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_CANCEL_METHOD)) { - // Final 2xx response, so break the linkage between the Sproutlets. - // For 3xx-6xx responses, the linkage between the sproutlets will be - // broken once the ACK to the response has been sent. _dmap_sproutlet.erase(i->second); _umap.erase(i); } @@ -1263,7 +1254,7 @@ const std::string& SproutletWrapper::service_name() const /// Returns a mutable clone of the original request suitable for forwarding /// or as the basis for constructing a response. -pjsip_msg* SproutletWrapper::original_request() +pjsip_msg* SproutletWrapper::get_request_for_sproutlet_tsx() { pjsip_tx_data* clone = PJUtils::clone_msg(stack_data.endpt, _req); @@ -1287,6 +1278,14 @@ pjsip_msg* SproutletWrapper::original_request() pj_list_erase(hr); } + // Decrement Max-Forwards if present. + pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) + pjsip_msg_find_hdr(clone->msg, PJSIP_H_MAX_FORWARDS, NULL); + if (mf_hdr != NULL) + { + --mf_hdr->ivalue; + } + register_tdata(clone); return clone->msg; @@ -1675,20 +1674,12 @@ void SproutletWrapper::rx_request(pjsip_tx_data* req) } // Clone the request to get a mutable copy to pass to the Sproutlet. - pjsip_msg* clone = original_request(); + pjsip_msg* clone = get_request_for_sproutlet_tsx(); if (clone == NULL) { // @TODO } - // Decrement Max-Forwards if present. - pjsip_max_fwd_hdr* mf_hdr = (pjsip_max_fwd_hdr*) - pjsip_msg_find_hdr(clone, PJSIP_H_MAX_FORWARDS, NULL); - if (mf_hdr != NULL) - { - --mf_hdr->ivalue; - } - if (PJSIP_MSG_TO_HDR(clone)->tag.slen == 0) { TRC_VERBOSE("%s pass initial request %s to Sproutlet", @@ -1725,30 +1716,28 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) log_inter_sproutlet(rsp, false); } - int status_code = rsp->msg->line.status.code; - if (status_code == PJSIP_SC_TRYING) + if ((status_code == PJSIP_SC_TRYING) || + (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD)) { _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); } - else if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id != PJSIP_CANCEL_METHOD) + else { _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, false); } - // If this is a negative response, send an immediate ACK response. - if (status_code >= 300) + // If this is a negative response to an INVITE, send an immediate ACK + // to the downstream sproutlet. + if ((status_code >= 300) && + (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_INVITE_METHOD)) { TRC_DEBUG("Send immediate ACK to negative response"); tx_negative_ack(rsp, fork_id); } - // If this is a 200 OK to a CANCEL, we don't want to do anything with it, so - // just notify the sproutlet that it has been received and then drop our - // reference to it. if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD) { - _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true); pjsip_tx_data_dec_ref(rsp); } else diff --git a/src/ut/mock_sproutlet.h b/src/ut/mock_sproutlet.h index edf241271..56dc51ba9 100644 --- a/src/ut/mock_sproutlet.h +++ b/src/ut/mock_sproutlet.h @@ -72,11 +72,11 @@ class MockSproutletTsx : public SproutletTsx MOCK_METHOD1(on_rx_initial_request, void(pjsip_msg*)); MOCK_METHOD1(on_rx_in_dialog_request, void(pjsip_msg*)); - MOCK_METHOD2(obs_rx_request, void(pjsip_msg*)); - MOCK_METHOD2(obs_tx_request, void(pjsip_msg*, int)); + MOCK_METHOD2(obs_rx_request, void(pjsip_msg*, bool)); + MOCK_METHOD3(obs_tx_request, void(pjsip_msg*, int, bool)); MOCK_METHOD2(on_rx_response, void(pjsip_msg*, int)); - MOCK_METHOD1(obs_rx_response, void(pjsip_msg*, int)); - MOCK_METHOD1(obs_tx_response, void(pjsip_msg*)); + MOCK_METHOD3(obs_rx_response, void(pjsip_msg*, int, bool)); + MOCK_METHOD2(obs_tx_response, void(pjsip_msg*, bool)); MOCK_METHOD2(on_rx_cancel, void(int, pjsip_msg*)); MOCK_METHOD1(on_timer_expiry, void(void*)); }; diff --git a/src/ut/mocktsxhelper.h b/src/ut/mocktsxhelper.h index 610b3d63a..fe32b7466 100644 --- a/src/ut/mocktsxhelper.h +++ b/src/ut/mocktsxhelper.h @@ -56,7 +56,7 @@ class MockSproutletTsxHelper : public SproutletTsxHelper SAS::TrailId trail() const {return _trail;} SAS::TrailId _trail; - MOCK_METHOD0(original_request, pjsip_msg*()); + MOCK_METHOD0(get_request_for_sproutlet_tsx, pjsip_msg*()); MOCK_CONST_METHOD0(route_hdr, const pjsip_route_hdr*()); MOCK_CONST_METHOD1(get_reflexive_uri, pjsip_sip_uri*(pj_pool_t*)); MOCK_CONST_METHOD1(is_uri_reflexive, bool(const pjsip_uri*)); From 25d456af20f966972d5d29922639dba991017f52 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Fri, 3 Mar 2017 16:13:54 +0000 Subject: [PATCH 15/17] Update comment --- src/sproutletproxy.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 0132b1c79..8e0253906 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -2014,8 +2014,8 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) return; } - // Sproutlets don't forward 100 Trying responses so we should never hit this - // code. But let's keep it here just in case. + // Sproutlets shouldn't forward 100 Trying responses but they might do it + // anyway, so let's keep this code. if (status_code == 100) { // We will already have sent a locally generated 100 Trying response, so From 1924651a69b845b543e687bd08b4c92c001c5fc5 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Mon, 6 Mar 2017 08:57:16 +0000 Subject: [PATCH 16/17] Add back scheduling code --- src/sproutletproxy.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 8e0253906..48309f614 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -625,6 +625,9 @@ void SproutletProxy::UASTsx::process_cancel_request(pjsip_rx_data* rdata) // Pass the CANCEL to the Sproutlet at the root of the tree. pjsip_tx_data* tdata = PJUtils::clone_msg(stack_data.endpt, rdata); _root->rx_cancel(tdata); + + // Schedule any requests generated by the Sproutlet. + schedule_requests(); } } From 153001787806de8e726b07bc8bcecde4fc4dcc90 Mon Sep 17 00:00:00 2001 From: Sathiyan Sivathas Date: Tue, 14 Mar 2017 16:22:22 +0000 Subject: [PATCH 17/17] Don't allow 100 Trying forwarding --- src/sproutletproxy.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/sproutletproxy.cpp b/src/sproutletproxy.cpp index 48309f614..37cf2ee1f 100644 --- a/src/sproutletproxy.cpp +++ b/src/sproutletproxy.cpp @@ -1488,6 +1488,12 @@ void SproutletWrapper::send_response(pjsip_msg*& rsp) return; } + if (rsp->line.status.code == PJSIP_SC_TRYING) + { + TRC_ERROR("Sproutlet attempted to forward a 100 Trying response"); + return; + } + TRC_VERBOSE("%s sending %s", _id.c_str(), pjsip_tx_data_get_info(it->second)); // We've found the tdata, move it to _send_responses. @@ -1786,6 +1792,8 @@ void SproutletWrapper::rx_response(pjsip_tx_data* rsp, int fork_id) if (status_code == PJSIP_SC_TRYING) { _sproutlet_tsx->on_rx_trying(rsp->msg, fork_id); + deregister_tdata(rsp); + pjsip_tx_data_dec_ref(rsp); } else { @@ -2017,18 +2025,6 @@ void SproutletWrapper::aggregate_response(pjsip_tx_data* rsp) return; } - // Sproutlets shouldn't forward 100 Trying responses but they might do it - // anyway, so let's keep this code. - if (status_code == 100) - { - // We will already have sent a locally generated 100 Trying response, so - // don't forward this one. - TRC_DEBUG("Discard 100/INVITE response (%s)", rsp->obj_name); - deregister_tdata(rsp); - pjsip_tx_data_dec_ref(rsp); - return; - } - if ((status_code > 100) && (status_code < 199)) {