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

[Joshua Colp]

> On Oct. 15, 2014, 8:37 p.m., Mark Michelson wrote:
> > /trunk/res/res_pjsip_keepalive.c, lines 151-153
> > <https://reviewboard.asterisk.org/r/4084/diff/1/?file=68351#file68351line151>
> >
> >     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?

It is not.


> On Oct. 15, 2014, 8:37 p.m., Mark Michelson wrote:
> > /trunk/res/res_pjsip_keepalive.c, lines 92-95
> > <https://reviewboard.asterisk.org/r/4084/diff/1/?file=68351#file68351line92>
> >
> >     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.

I can't guarantee that would be safe to do because the raw method still stores some attempt specific information on the tdata and has a callback. Provided the callback happens within the call it could be fine, but if it happens later OR allocates any additional data from the tdata pool then I'm back at the beginning. I'd have to use an individual tdata and reset the pool and recreate state.


> On Oct. 15, 2014, 8:37 p.m., Mark Michelson wrote:
> > /trunk/res/res_pjsip_keepalive.c, line 152
> > <https://reviewboard.asterisk.org/r/4084/diff/1/?file=68351#file68351line152>
> >
> >     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?

Confirmed, there be no assertions that would occur.


- Joshua


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


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/20141016/d17ad9f4/attachment-0001.html>