[Asterisk-code-review] res_pjsip_pubsub: destroy old subscriptions on RLS update using seria... (asterisk[16])

[Alexei Gradinari]

Attention is currently required from: Joshua Colp, N A.
Alexei Gradinari has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18891 )

Change subject: res_pjsip_pubsub: destroy old subscriptions on RLS update using serializer
......................................................................


Patch Set 2:

(1 comment)

File res/res_pjsip_pubsub.c:

https://gerrit.asterisk.org/c/asterisk/+/18891/comment/9aa37d47_8c12ea99 
PS2, Line 4084: 						if (ast_sip_push_task(sub_tree->serializer, destroy_subscriptions_task, old_root)) {
> Under what case would this not already be executing under the serializer of the sub_tree? The dialog […]
This is RLS update which happens in pubsub_on_rx_refresh.

I could not find that pubsub_on_rx_refresh is executed by sub_tree->serializer.
But I had the sigfaults in res_pjsip_exten_state.c:notify_task 
a few times after RLS was updated.

I thought the following happened:
The asterisk checked if there were changes to the list on the subscriptions refresh
by generating a new ast_sip_subscription tree and comparing with old ast_sip_subscription tree.
There was difference and the asterisk replaced root with new tree and destroyed old tree.
At the same time when the old tree was destroying the res_pjsip_exten_state.c:notify_task was executing by sub_tree->serializer and part of data (datastore) was already destroyed.
The sigfault was in ast_datastores_find which is called by res_pjsip_dialog_info_body_generator.

So I thought that pubsub_on_rx_refresh was not executed by sub_tree->serializer.

Since I implemented this fix, I have not seen any more sigfaults on RLS update.

Maybe it's not because I put the destruction task into the serializer, but because set_state_terminated(old_root) is called before the destruction task.

If you are sure that pubsub_on_rx_refresh is executed by sub_tree->serializer,
I'll remove ast_sip_push_task and leave only set_state_terminated(old_root).

What do you think?



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18891
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I96582aad3a26515ca73a8460ee6756f56f6ba23b
Gerrit-Change-Number: 18891
Gerrit-PatchSet: 2
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Tue, 16 Aug 2022 14:51:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220816/4cbb06b0/attachment-0001.html>