[asterisk-dev] [Code Review] 4084: res_pjsip_keepalive: Add keepalive module for connection-oriented transports.

[Mark Michelson]

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


You have red blobs on lines 164, 188, and 271.


/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24053>

    Possible optimization:
    
    Since tpmgr is having to create the exact same tdata every time you send a keepalive, it may be worth creating and storing your own tdata instead. Then you can just reuse the same tdata each time you send a keepalive.
    
    Probably not worth doing unless we determine that there really is an issue here, but worth noting nonetheless.



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24047>

    Since keepalive_transport_cb() always returns 0, I don't think that OBJ_MULTIPLE is required here. OBJ_MULTIPLE is only necessary if multiple calls to the callback may return CMP_MATCH.



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24051>

    I'm not very familiar with the PJSIP transport state machine, but I can see that there are two additional states, PJSIP_TP_STATE_SHUTDOWN and PJSIP_TP_STATE_DESTROY. Is it possible for a connected transport to enter one of those two states without first entering the disconnected state?



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24052>

    Hm, seems like there may be a race condition here, but it may be benign. If a connection is disconnected, then the call to ao2_find() to remove the transport from the keepalive container may block until the current set of keepalives has completed being sent.
    
    While the keepalives are being sent, it is possible that we may try to send a keepalive on the transport that just disconnected. That may just result in an error return from pjsip_tpmgr_send_raw(), but there's also that worry it could trip an assertion. Can you verify?



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24050>

    You can simplify this to
    
    return ast_sorcery_generic_alloc(sizeof(*configuration), NULL);



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24046>

    The if here is unnecessary. The only way this can be reached is if configuration->interval is 0. You can just use a bare else.



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24048>

    I had a look at the documentation for this function, and PJSIP has this to say:
    
    "Note that this function will override the existing callback, if any, so application is recommended to keep the old callback and manually forward the notification to the old callback, otherwise other component that concerns about the transport state will no longer receive transport state events."
    
    I have no idea what, if anything would have set a tpmgr callback, but it's probably a good idea to follow the advice given here. You can get the current callback by calling pjsip_tpmgr_get_state_cb(). You'll probably have to store this callback in a static variable in this file since you cannot pass user data to your own tpmgr callback. Then in your tpmgr callback, you can call the old callback before returning.



/trunk/res/res_pjsip_keepalive.c
<https://reviewboard.asterisk.org/r/4084/#comment24049>

    Put the appropriate support level in this structure.


- Mark Michelson


On Oct. 15, 2014, 6:08 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4084/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2014, 6:08 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Reusing connections is a good thing(tm). In the case of NAT it means that you have an actual way to exchange data back and forth. In practice, however, some things (such as PJSIP) close down the TCP connection after a short period of time (in the case of PJSIP for UAC it's 33 seconds). While PJSIP has a built in keepalive mechanism this is by default set to 90 seconds and can only be controlled at compile time.
> 
> The attached module implements its own keepalive which is configurable at runtime and does not require configuration. This sends a lightweight keepalive to keep the TCP (or TLS) connection open.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_keepalive.c PRE-CREATION 
>   /trunk/configs/samples/pjsip.conf.sample 425216 
> 
> Diff: https://reviewboard.asterisk.org/r/4084/diff/
> 
> 
> Testing
> -------
> 
> Configured keepalives and used wireshark to verify that at the specific interval the message went out.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141015/7726dd6a/attachment-0001.html>