[Asterisk-code-review] res pjsip pubsub: Add more locking and checking around NOTIFY (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: res_pjsip_pubsub: Add more locking and checking around NOTIFY
......................................................................


Patch Set 4: Code-Review-1

Some of the changes presented in this review are not necessary.

First, I double-checked and the dialog is actually locked on all calls into pubsub_on_evsub_state(). There are four ways that the evsub code can call into us:
* On subscription timeout
* On direct subscription termination
* When sending a request
* When transaction state changes

For the first three, the evsub code locks the dialog directly before calling into us. For the fourth, it's a bit harder to trace, but the dialog is actually locked before reaching the evsub code. Here's an example of a backtrace I took:

#0: [0x7fbc8671a5ee] res/res_pjsip_pubsub.c:3404 pubsub_on_evsub_state()
#1: [0x7fbcac20f988] evsub.c:0 set_state()
#2: [0x7fbcac210ff9] evsub.c:0 mod_evsub_on_tsx_state()
#3: [0x7fbc87be1fa5] :0 pjsip_dlg_on_tsx_state() (0x7fbc87be1f30+75)
#4: [0x7fbc87bdcd79] sip_transaction.c:0 tsx_set_state()
#5: [0x7fbc87bdda5e] sip_transaction.c:0 tsx_on_state_null()
#6: [0x7fbc87bdf95f] :0 pjsip_tsx_recv_msg() (0x7fbc87bdf8e0+7F)
#7: [0x7fbc87be1cbf] :0 pjsip_dlg_on_rx_request() (0x7fbc87be1bb0+10F)
#8: [0x7fbc87be32e3] sip_ua_layer.c:0 mod_ua_on_rx_request()
#9: [0x7fbc87bcac87] :0 pjsip_endpt_process_rx_data() (0x7fbc87bcab40+147)
#10: [0x7fbcac4436c9] res_pjsip/pjsip_distributor.c:766 distribute()
#11: [0x5f5ac1] main/taskprocessor.c:940 ast_taskprocessor_execute() (0x5f59b4+10D)
#12: [0x5ff259] main/threadpool.c:1322 execute_tasks()
#13: [0x5f5ac1] main/taskprocessor.c:940 ast_taskprocessor_execute() (0x5f59b4+10D)
#14: [0x5fd513] main/threadpool.c:351 threadpool_execute()
#15: [0x5feb55] main/threadpool.c:1105 worker_active()
#16: [0x5fe90e] main/threadpool.c:1025 worker_start()
#17: [0x60b128] main/utils.c:1235 dummy_start()

The dialog is locked in frame #3. The takeaway here is that the transaction user is the dialog layer, not evsub. The transaction layer calls into the dialog layer directly on transaction state changes. The dialog layer then passes the transaction state change to its dialog usages. All transaction state changes (including transaction timeout) will lock the dialog because of this structuring.

So with all four paths covered, the dialog will be locked in all cases before being called into by the evsub code.


The other change that is not necessary is the added serialization in ast_sip_subscription_notify(). Currently, there are two callers of this function, one in res_pjsip_mwi.c and one in res_pjsip_exten_state.c.

The res_pjsip_exten_state.c usage is straightforward. You can see that it pushes a notify_task() onto the subscription's serializer. That notify_task() is what then calls ast_sip_subscription_notify().

The res_pjsip_mwi.c usage is a bit less easy to follow. The reason is because there exist both solicited and unsolicited NOTIFYs for MWI. Solicited MWIs have a subscription associated with them and therefore push a serialized_notify() onto the subscription serializer. ast_sip_subscription_notify() is then called from the subscription serializer. For unsolicited MWI notifications, there is no corresponding subscription object. Therefore, the serialized_notify() is pushed to an arbitrary serializer. Furthermore, because the NOTIFY is unsolicited, we don't even call ast_sip_subscription_notify() on this path. We instead just send an out-of-dialog request to the endpoint.

-- 
To view, visit https://gerrit.asterisk.org/3018
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I779d11802cf672a51392e62a74a1216596075ba1
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No