[asterisk-dev] [Code Review] 4193: Stasis: allow for subscriptions to dictate message delivery on a threadpool for certain situations

[Matt Jordan]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4193/
-----------------------------------------------------------

(Updated Nov. 24, 2014, 12:07 p.m.)


Review request for Asterisk Developers.


Changes
-------

Addressed Josh's finding and added configuration options. This is partly a backport of the existing stasis.conf from Asterisk 13 (minus the actual 'settings'), and partly a pull-in of the original stasis.conf configuration parsing provided on https://reviewboard.asterisk.org/r/2881.

Addressed Mark's finding by removing the unneeded option and by making the message routers in app_queue use the threadpool as well.


Bugs: ASTERISK-24533
    https://issues.asterisk.org/jira/browse/ASTERISK-24533


Repository: Asterisk


Description
-------

Note: don't be fooled by the size of this review. Most of the changes are small and are in stasis.c/stasis_message_router.c.

Rob (and CDR on the asterisk-users list, although he never followed up when we asked for more information) discovered that we were creating two additional threads per SIP peer in chan_sip. This actually would occur for a large variety of 'endpoints' in Asterisk, regardless of the channel driver. The two threads were stasis subscriptions, specifically for MWI and for endpoints detecting the destruction of related channels.

Stasis subscriptions currently get a dedicated thread. In contrast, prior to r400178 (see review https://reviewboard.asterisk.org/r/2881/), the subscriptions shared a thread pool. It was discovered that for a low subscription count with high message throughput, the threadpool was not as performant as simply having a dedicated thread per subscriber.

Since then, our subscription pattern has changed slightly. While we still have plenty of subscriptions that would follow the model just described (AMI, CDRs, CEL, etc.) - there are plenty that also fall into the following two categories:
* Large number of subscriptions, specifically those tied to endpoints/peers.
* Low number of messages. Some subscriptions exist specifically to coordinate a single message - the subscription is created, a message is published, the delivery is synchronized, and the subscription is destroyed.
In both of the latter two cases, creating a dedicated thread is wasteful (and in the case of a large number of peers/endpoints, harmful).

This patch adds the ability of a subscriber to Stasis to choose whether or not their messages are dispatched on a dedicated thread or on a threadpool. The threadpool is currently hard coded in this patch; if configuration is necessary, we could re-add the configuration options in r2881. Pre-mature optimization and all that led me to simply go with a simpler model for now.

Note that the approach taken here was to add additional API calls rather than modify existing ones. If we'd rather the thread dispatch model be dictated by a parameter, the new API calls can be removed in trunk and the old API calls modified appropriately.


Diffs (updated)
-----

  /team/mjordan/12-threadpool/tests/test_stasis.c 428600 
  /team/mjordan/12-threadpool/res/res_xmpp.c 428600 
  /team/mjordan/12-threadpool/res/res_stasis_device_state.c 428600 
  /team/mjordan/12-threadpool/res/res_pjsip_refer.c 428600 
  /team/mjordan/12-threadpool/res/res_pjsip_pubsub.c 428600 
  /team/mjordan/12-threadpool/res/res_pjsip_mwi.c 428600 
  /team/mjordan/12-threadpool/res/res_jabber.c 428600 
  /team/mjordan/12-threadpool/res/parking/parking_bridge_features.c 428600 
  /team/mjordan/12-threadpool/res/parking/parking_applications.c 428600 
  /team/mjordan/12-threadpool/main/stasis_message_router.c 428600 
  /team/mjordan/12-threadpool/main/stasis_channels.c 428600 
  /team/mjordan/12-threadpool/main/stasis_cache.c 428600 
  /team/mjordan/12-threadpool/main/stasis.c 428600 
  /team/mjordan/12-threadpool/main/endpoints.c 428600 
  /team/mjordan/12-threadpool/include/asterisk/stasis_message_router.h 428600 
  /team/mjordan/12-threadpool/include/asterisk/stasis_internal.h 428600 
  /team/mjordan/12-threadpool/include/asterisk/stasis.h 428600 
  /team/mjordan/12-threadpool/configs/stasis.conf.sample PRE-CREATION 
  /team/mjordan/12-threadpool/channels/sig_pri.c 428600 
  /team/mjordan/12-threadpool/channels/chan_skinny.c 428600 
  /team/mjordan/12-threadpool/channels/chan_sip.c 428600 
  /team/mjordan/12-threadpool/channels/chan_mgcp.c 428600 
  /team/mjordan/12-threadpool/channels/chan_iax2.c 428600 
  /team/mjordan/12-threadpool/channels/chan_dahdi.c 428600 
  /team/mjordan/12-threadpool/apps/app_queue.c 428600 

Diff: https://reviewboard.asterisk.org/r/4193/diff/


Testing (updated)
-------

New unit tests were written to cover the new subscription types. This tests receiving messages sent using the threadpool.

The MWI tests and channel driver tests in the Test Suite were run and passed.

For the new configuration options:
* Tested under valgrind; confirmed no memory leaks with/without the config file
* Tested that the lack of stasis.conf did not preclude loading and that the defaults were applied
* Tested with an invalid config file; defaults still applied
* Tested with a valid config file; custom values applied


Thanks,

Matt Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141124/daaa41bc/attachment.html>