<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7710">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">Overall, I like the layered status compositor approach and the pre-setup links between the layers. It makes the normal status update path faster by stopping updates going higher if they cannot change the next layer's status report. It also makes the normal path faster because the next higher layer does not need to be found each time.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Updates to the sip_options_aor layer structure seem to be better controlled because the critical work is done by the sip_options_aor serializer to keep the available status accurate.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, how the relationship of the endpoint and sip_options_aor layers is updated is currently quite fragile. The order of observer callbacks between endpoint and aor objects is indeterminate as they are executed by different serializers. The rebuilding and update of the status compositor reporting tree is problematic as a result. The endpoint and aor layers need to be as independent as possible. There doesn't seem to be enough protection from normal status updates interfering with the restructuring. I think it would be much simpler for the endpoint compositor's aor container to also keep the last sip_options_aor available status contribution to make the endpoint compositor available status layer and the sip_options_aor available status layer more independent.</p><p>(46 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7710/1/funcs/func_pjsip_contact.c">File funcs/func_pjsip_contact.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/funcs/func_pjsip_contact.c@147">Patch Set #1, Line 147:</a> <code style="font-family:monospace,monospace"> contact_status = ast_sip_get_contact_status(contact_obj);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is no protection from contact_status == NULL (Pre-existing)</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7710/1/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/include/asterisk/res_pjsip.h@264">Patch Set #1, Line 264:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! If true authenticate the qualify if needed */<br> int authenticate_qualify;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">contact authenticate_qualify is passed around but not used for anything (Pre-existing)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/include/asterisk/res_pjsip.h@303">Patch Set #1, Line 303:</a> <code style="font-family:monospace,monospace">struct ast_sip_contact_status {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The contact_status sorcery object needs to be kept to not change the API. A third party may have setup an observer. Mitigating factors are that we never declared the table in alembic so nobody should have a realtime table setup. Only third party modules may have setup an observer. Not sure how much we want to care though.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/include/asterisk/res_pjsip.h@342">Patch Set #1, Line 342:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! If true authenticate the qualify if needed */<br> int authenticate_qualify;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">aor authenticate_qualify is passed around but not used for anything (Pre-existing)</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/location.c">File res/res_pjsip/location.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/location.c@1252">Patch Set #1, Line 1252:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_sorcery_object_set_congestion_levels(sorcery, "contact", -1,<br> 3 * AST_TASKPROCESSOR_HIGH_WATER_LEVEL);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should be able to use the default congestion levels for the sorcery/contact serializer with this rewrite.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_configuration.c@44">Patch Set #1, Line 44:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! \brief AORs that we should react to */<br> char *aors;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">aors no longer referenced</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_configuration.c@1115">Patch Set #1, Line 1115:</a> <code style="font-family:monospace,monospace">int ast_sip_persistent_endpoint_publish_contact_state(const char *endpoint_name, const struct ast_sip_contact_status *contact_status)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Return void. The only caller does not care about the return value. It only wants to publish the contact status for each endpoint it knows about. What could any caller do about a failed publish anyway?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c">File res/res_pjsip/pjsip_options.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@102">Patch Set #1, Line 102:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! \brief The names of the AORs feeding this compositor */<br> struct ao2_container *aors;<br> /*! \brief The number of AORs that are available */<br> unsigned int available;<br> /*! \brief The name of the endpoint */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Keeping the total available aor count correct is fragile.</p><p style="white-space: pre-wrap; word-wrap: break-word;">More thought on structure updates from observers is needed. I think it would be much simpler for the compositor's aor container to also keep the last sip_options_aor available status contribution to make the endpoint compositor available status layer and the sip_options_aor available status layer more independent.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@122">Patch Set #1, Line 122:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! \brief The endpoint state compositors we are feeding */<br> AST_VECTOR(, struct sip_options_endpoint_state_compositor *) compositors;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does it make sense for multiple endpoints to reference the same aor?<br>A vector is not needed if it doesn't. Though, we might have two endpoints temporarily while a config loads.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a note that the vector holds a ref to each compositor</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@128">Patch Set #1, Line 128:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! \brief If true authenticate the qualify if needed */<br> int authenticate_qualify;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">authenticate_qualify value is not needed. We send OPTIONS not receive them.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@364">Patch Set #1, Line 364:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!sip_options_contact_statuses) {<br> sip_options_contact_statuses = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, CONTACT_BUCKETS,<br> sip_contact_status_hash, NULL, sip_contact_status_cmp);<br> if (!sip_options_contact_statuses) {<br> return NULL;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Put this into a helper routine so the container is always created the same way by the two locations that can create it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@479">Patch Set #1, Line 479:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_FREE(&aor_options->compositors);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should add an assert here to ensure that the vector is empty.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@485">Patch Set #1, Line 485:</a> <code style="font-family:monospace,monospace">static int sip_contact_hash(const void *obj, const int flags)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">sip_contact_hash replaced by int ast_sorcery_object_id_hash(const void *obj, int flags)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@506">Patch Set #1, Line 506:</a> <code style="font-family:monospace,monospace">static int sip_contact_cmp(void *obj, void *arg, int flags)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">sip_contact_cmp replaced by int ast_sorcery_object_id_compare(void *obj, void *arg, int flags)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@613">Patch Set #1, Line 613:</a> <code style="font-family:monospace,monospace"> if (ao2_container_count(endpoint_state_compositor->aors)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It maybe better if we search for the aor name in the aors container rather than seeing if the container is empty.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@927">Patch Set #1, Line 927:</a> <code style="font-family:monospace,monospace"> aor_options->serializer = ast_sip_create_serializer_named(tps_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to use ast_sip_create_serializer_group_named() to create the serializer</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@979">Patch Set #1, Line 979:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,<br> "+1", 1.0, ast_sip_get_contact_status_label(contact_status->status));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should remove this. Nothing brings the REMOVED contact status statsd guage down except asterisk shutting down. Temporary contacts from rewrite_contact increase this count. (Pre-existing)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1043">Patch Set #1, Line 1043:</a> <code style="font-family:monospace,monospace"> int new)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Change new to is_new to avoid using C++ keywords for variables.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1055">Patch Set #1, Line 1055:</a> <code style="font-family:monospace,monospace"> existing_permanent_contacts = ao2_container_clone(aor_options->permanent_contacts, 0);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It seems like this could be expensive? It can be optimized away if (!aor-></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The clone shouldn't be that expensive as there won't be many items in the container.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The container must exist as the sip_options_aor allocation fails if the container couldn't be created.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The locking concern is not a problem as manipulation of the sip_options_aor structure is done by the struct's serializer. Only one thread can manipulate the struct instance at a time. Besides, the container was intentionally created without a lock.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1071">Patch Set #1, Line 1071:</a> <code style="font-family:monospace,monospace"> ao2_find(existing_permanent_contacts, ast_sorcery_object_get_id(contact), OBJ_NODATA | OBJ_UNLINK);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add OBJ_SEARCH_KEY to ao2_find here</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1103">Patch Set #1, Line 1103:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> aor_options->available = ao2_container_count(aor_options->dynamic_contacts) +<br> ao2_container_count(aor_options->permanent_contacts);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">All the permanent and dynamic contact statuses need to be set to created/unknown.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1108">Patch Set #1, Line 1108:</a> <code style="font-family:monospace,monospace"> aor_options->available = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All the permanent and dynamic contact statuses need to be set to created/unknown/unavailable so the OPTIONS ping will increment the available count.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1125">Patch Set #1, Line 1125:</a> <code style="font-family:monospace,monospace"> /* If there is still a qualify frequency then schedule this */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">aor_options->qualify_frequency should be updated here to avoid sched thread fetching a partially set value.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1138">Patch Set #1, Line 1138:</a> <code style="font-family:monospace,monospace"> aor_options->qualify_frequency = aor->qualify_frequency;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">aor_options->qualify_frequency should not be set here</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1139">Patch Set #1, Line 1139:</a> <code style="font-family:monospace,monospace"> aor_options->qualify_timeout = aor->qualify_timeout;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">aor_options->qualify_timeout should be updated before rescheduling the OPTIONS qualify</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1140">Patch Set #1, Line 1140:</a> <code style="font-family:monospace,monospace"> aor_options->authenticate_qualify = aor->authenticate_qualify;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">options->authenticate_qualify is set but not used</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1348">Patch Set #1, Line 1348:</a> <code style="font-family:monospace,monospace"> ast_sip_persistent_endpoint_update_state(ast_sorcery_object_get_id(endpoint), AST_ENDPOINT_OFFLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be deferred until after the loop. If we don't have a compositor at that point then set endpoint to offline</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1398">Patch Set #1, Line 1398:</a> <code style="font-family:monospace,monospace"> for (i = 0; i < AST_VECTOR_SIZE(&aor_options->compositors); ++i) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The processing of the compositors vector should be done under the aor_options serializer.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also we should reset the compositors vector after the loop or remove the elements in the loop.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1404">Patch Set #1, Line 1404:</a> <code style="font-family:monospace,monospace"> endpoint_state_compositor->aors = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">endpoint_state_compositor->aors is leaked here. Need to ast_str_container_remove(endpoint_state_compositor->aors, aor_options->name).</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1405">Patch Set #1, Line 1405:</a> <code style="font-family:monospace,monospace"> endpoint_state_compositor->available = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should be decrementing the available count if this aor was available before. The endpoint may have more than one aor contributor.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1422">Patch Set #1, Line 1422:</a> <code style="font-family:monospace,monospace"> if (endpoint_state_compositor->aors) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to check if endpoint_state_compositor->aors is not an empty container instead</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1492">Patch Set #1, Line 1492:</a> <code style="font-family:monospace,monospace"> ast_sip_push_task_synchronous(management_serializer, sip_options_synchronize_task, &task_data);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_sip_push_task_synchronous() can fail. If so then nothing cleans up the passed in task_data if needed. Needs to be done throughout the file.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1512">Patch Set #1, Line 1512:</a> <code style="font-family:monospace,monospace"> for (; (aor = ao2_iterator_next(&it_aors)); ao2_ref(aor, -1)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The compositor lock needs to be held while finding the next compositor->aor since the compositor->aor container has no lock</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1557">Patch Set #1, Line 1557:</a> <code style="font-family:monospace,monospace">static const struct ast_sorcery_observer endpoint_observer_callbacks = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The order of observer callbacks between endpoint and aor objects is indeterminate as they are executed by different serializers. The rebuilding and update of the status compositor reporting tree is problematic as a result.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The endpoint observers must rebuild the endpoint-aor links because they know the relationships. If an added/updated endpoint references an aor that actually exists but doesn't have a sip_options_aor object yet then a stub object needs to be created until the aor observer callback can add/update the real object into the tree.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The aor observers need to add/update/delete the sip_options_aor objects and update their status to any linked endpoint compositors.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1660">Patch Set #1, Line 1660:</a> <code style="font-family:monospace,monospace"> for (i = 0; i < AST_VECTOR_SIZE(&aor_options->compositors); ++i) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The processing of the compositors vector should be done under the aor_options serializer.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also we should reset the compositors vector after the loop or remove the elements in the loop.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1723">Patch Set #1, Line 1723:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* If this is the first contact we need to schedule up qualification */<br> if ((ao2_container_count(task_data->aor_options->dynamic_contacts) + ao2_container_count(task_data->aor_options->permanent_contacts)) == 1) {<br> ast_debug(3, "Starting scheduled callback on AOR '%s' for qualifying as there is now a contact on it\n",<br> task_data->aor_options->name);<br> /* We immediately schedule the initial qualify so that we get reachable/unreachable as soon as possible.<br> * Realistically since they pretty much just registered they should be reachable.<br> */<br> task_data->aor_options->sched_id = ast_sched_add_variable(sched, 1, sip_options_qualify_aor,<br> ao2_bump(task_data->aor_options), 1);<br> if (task_data->aor_options->sched_id < 0) {<br> ao2_t_ref(task_data->aor_options, -1, "Cleanup failed scheduler add");<br> ast_log(LOG_ERROR, "Unable to schedule qualify for contacts of AOR '%s'\n", task_data->aor_options->name);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We need to push the start onto the management_serializer to avoid a race condition that could start the scheduler twice.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1790">Patch Set #1, Line 1790:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if ((ao2_container_count(task_data->aor_options->dynamic_contacts) + ao2_container_count(task_data->aor_options->permanent_contacts)) == 0) {<br> ast_debug(3, "Terminating scheduled callback on AOR '%s' as there are no contacts to qualify\n",<br> task_data->aor_options->name);<br> AST_SCHED_DEL_UNREF(sched, task_data->aor_options->sched_id, ao2_t_ref(task_data->aor_options, -1, "Delete scheduler entry ref"));<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stoping the scheduler here can deadlock. Need to push the stopping onto the management_serializer to avoid deadlock and other race conditions with the scheduler.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2009">Patch Set #1, Line 2009:</a> <code style="font-family:monospace,monospace"> ast_sched_context_destroy(sched);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Any scheduled AOR qualify ping sip_options_aor ref is leaked here. Need to push a stopping of all scheduled items task to the management_serializer. Also need a shutdown flag.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2012">Patch Set #1, Line 2012:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_sorcery_observer_remove(ast_sip_get_sorcery(), "contact", &contact_observer_callbacks);<br> ast_sorcery_observer_remove(ast_sip_get_sorcery(), "aor", &aor_observer_callbacks);<br> ast_sorcery_observer_remove(ast_sip_get_sorcery(), "endpoint", &endpoint_observer_callbacks);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to uninstall the observer callbacks before stopping the scheduler.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2015">Patch Set #1, Line 2015:</a> <code style="font-family:monospace,monospace"> ast_taskprocessor_unreference(management_serializer);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The management_serializer needs to be shutdown after the ao2_cleanup() of the global containers</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2016">Patch Set #1, Line 2016:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ao2_cleanup(sip_options_aors);<br> ao2_cleanup(sip_options_contact_statuses);<br> ao2_cleanup(sip_options_endpoint_state_compositors);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to set these global containers to NULL after ao2_cleanup()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2019">Patch Set #1, Line 2019:</a> <code style="font-family:monospace,monospace"> pjsip_endpt_unregister_module(ast_sip_get_pjsip_endpoint(), &options_module);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Insert the following before unregistering the module:</p><ul><li>Need to send a final flush task to the management_serializer here to destroy it.</li><li>Need to wait for the management_serializer and the options_aor serializers to shutdown using ast_serializer_shutdown_group_join().</li><li>Need to destroy the scheduler here</li></ul></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2036">Patch Set #1, Line 2036:</a> <code style="font-family:monospace,monospace"> if (pjsip_endpt_add_capability(ast_sip_get_pjsip_endpoint(), NULL, PJSIP_H_ALLOW,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to create a shudown_group here using ast_serializer_shutdown_group_alloc()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2065">Patch Set #1, Line 2065:</a> <code style="font-family:monospace,monospace"> ast_taskprocessor_build_name(tps_name, sizeof(tps_name), "pjsip/options/manage");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is one and only one pjsip/options/manage serializer so no need to add a sequence number by calling ast_taskprocessor_build_name().</p><p style="white-space: pre-wrap; word-wrap: break-word;">Just pass the string to ast_sip_create_serializer_group_named(). The name cannot conflict with an aor named "manage" options serializer because that serializer name has a sequence number appended while the management_serializer will not.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2066">Patch Set #1, Line 2066:</a> <code style="font-family:monospace,monospace"> management_serializer = ast_sip_create_serializer_named(tps_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to use ast_sip_create_serializer_group_named() to create the serializer.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@2087">Patch Set #1, Line 2087:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> sched = ast_sched_context_create();<br> if (!sched) {<br> ast_res_pjsip_cleanup_options_handling();<br> return -1;<br> }<br><br> if (ast_sched_start_thread(sched)) {<br> ast_res_pjsip_cleanup_options_handling();<br> return -1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to start the scheduler before installing the observer callbacks and creating the management_serializer. The callbacks depend on the management_serializer and scheduler being already setup.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7710">change 7710</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/7710"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I6a5ebbfca9001dfe933eaeac4d3babd8d2e6f082 </div>
<div style="display:none"> Gerrit-Change-Number: 7710 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 14 Jan 2018 20:28:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>