Summary:	ASTERISK-10249: [patch] SIP Session-Timers Support in Asterisk
Reporter:	Raj Jain (rjain)	Labels:
Date Opened:	2007-09-07 03:43:46	Date Closed:	2008-01-16 15:51:51.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) AsteriskSipSessionTimers.pdf ( 1) chan_sip.c.1.diff ( 2) chan_sip.c.cache.diff ( 3) chan_sip.c.diff ( 4) chan_sip.memalloc ( 5) chan_sip.memalloc.bugfix ( 6) proc_422_rsp_comment.diff ( 7) sip.conf.sample.diff
Description:	The Asterisk SIP stack currently does not support SIP Session-Timers (RFC 4028). This leads to defunct SIP sessions in Asterisk when calls do not clear through normal signaling procedures due to network or end-point failures. John Todd recently discussed this concept on asterisk-dev: http://lists.digium.com/pipermail/asterisk-dev/2007-July/028574.html John Todd, JR Richardson and Kevin Fleming have expressed interest in seeing this feature supported in Asterisk. A software design document for this feature and code changes (unified diff of chan_sip.c) are attached to this report. Digium has my code submission agreement on file.
Comments:	By: John Todd (jtodd) 2007-09-07 10:48:47 I've tested against Sipura/Linksys/Cisco, Sylantro, and other Asterisk systems and it seems to do the right things. This code is work-for-hire sponsored by TalkPlus, Inc (John Todd) and Ntegratedsolutions (JR Richardson). By: Raj Jain (rjain) 2007-09-07 12:22:03 Uploaded chan_sip.c.1.diff which resolved a compiler warning. By: Ronald Chan (loloski) 2007-09-15 09:46:14 Any news here?, can someone from our developers to take a look at this? I think this is really important feature for possible inclusion in trunk. ping oej! Thanks By: John Todd (jtodd) 2007-10-01 15:35:28 At Astricon, there were several conversations about how good an idea this is, and how much it's needed.  I've tested on two systems, the developer (Raj) has tested on one.  No problems.  I'd like to get others to test this, and/or this to be stuffed into TRUNK for testing so we can get it into 1.6 since I believe this is a _very_ important patch for those of us that actually pay for our minutes.  Zombies are bad, mmmkay? By: Ronald Chan (loloski) 2007-10-01 17:46:33 rjain: I'm willing to test this on production system myself can you send us a backport version for 1.4? By: Digium Subversion (svnbot) 2007-10-02 07:59:59 Repository: asterisk Revision: 84362 A   team/group/sip_session_timers/ ------------------------------------------------------------------------ r84362 | russell | 2007-10-02 07:59:58 -0500 (Tue, 02 Oct 2007) | 4 lines Create branch for the code in issue ASTERISK-10249 to make it easier to test, review, and keep up to date with trunk.  (I will keep it up to date if there are any conflicts before it gets merged.) ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-10-02 08:17:37 Repository: asterisk Revision: 84364 U   team/group/sip_session_timers/channels/chan_sip.c ------------------------------------------------------------------------ r84364 | russell | 2007-10-02 08:17:36 -0500 (Tue, 02 Oct 2007) | 4 lines Merge patch from issue ASTERISK-10249, fixing the conflicts and API changes that have occurred in the past few weeks.  This branch is ready for use in testing this patch. ------------------------------------------------------------------------ By: Ronald Chan (loloski) 2007-10-02 10:09:47 Russel, Could you please post the link of your repository here. since i'm not very familiar with it. By: Ronald Chan (loloski) 2007-10-02 10:22:10 Nevermind i got it now, i will report my progress here.. svn co http://svn.digium.com/svn/asterisk/team/group/sip_session_timers/ By: Russell Bryant (russell) 2007-10-02 14:22:46 You can check out the branch that has this code by using the following command. $ svn co http://svn.digium.com/svn/asterisk/team/group/sip_session_timers Then, compile and install as usual.  Note that this branch is based off of trunk. By: Raj Jain (rjain) 2007-10-10 14:06:09 I tested the branch that Russell created (thanks!) against my test cases including a 5 calls/second regression test with session-timers enabled and things are working as expected (basically, no merge related issues). By: Ronald Chan (loloski) 2007-10-10 23:12:27 rjain: Could you please make a suggestion on how to test this on this topology UAC -> SIP -> *-UAS -> SIP -> *-UAS -> SIP ->UAC Can i yank the cable to test this ? heheh I'm really sorry i don't know the inner working of SIP in general Can someone put a config snippet on this topology? By: John Todd (jtodd) 2007-10-11 00:25:30 loloski : If you are testing failure modes, then you can disconnect the ethernet from any of the compnents and wait for the timeout (whatever you have it set to) and the call should disconnect.  Make sure you have Asterisk configured such that the media flows between endpoints, or if you do have RTP media flowing through the Asterisk servers, make sure you have the "rtptimeout" settings deactivated otherwise the call will hang up for reasons other than the session-timers. By: Raj Jain (rjain) 2007-10-11 00:53:11 loloski: Following are some sample parameters that you can put in your sip.conf in both your Asterisk boxes: session-timers=originate session-expires=120 session-minse=90 session-refresher=uas These will cause SIP sessions to/from Asterisk to be refreshed every 60 seconds. You can disconnect ethernet (as JT suggested) or kill one or more components abruptly while calls are active and notice that calls are released when the next session refresh happens. By: John Todd (jtodd) 2007-10-11 01:16:02 Actually, I was a bit too broad in my previous statement.  You can disconnect the ethernet from any device OTHER than the Asterisk systems.  The Asterisk boxes should remain plugged into a hub or switch or other system which continues to provide a carrier signal.  I don't know what * will do if the ethernet drivers detect a loss of signal and takes down the ethernet interface; perhaps it will interfere with the test results.  So just disconnect the SIP UAC systems at the ends of the call from their local ethernet port on the switch/hub. By: Ronald Chan (loloski) 2007-10-12 09:13:56 rjain rjain: thanks for your config sample, yes it works nicely when i yank the cable of one of the UAC going towards one of the UAS. Great Job!! n jtodd: thanks for sponsoring this great code another nice contribution to asterisk code base. Thanks By: Raj Jain (rjain) 2007-10-14 15:05:47 Ran into a minor bug when testing with Windows Messenger (a SIP end-point that does not support Session-Timers). We were calling start_session_timer() when the UAS does not support session-timers and we were configured in the "accept" mode. This doesn't really affect anything as the session-timer doesn't get started and the call proceeds as normal. Its just there was an unnecessary call to start_session_timer(). The fix for this issue has been uploaded as chan_sip.c.diff. The diff also addresses a code indentation issue. Also uploaded is sip.conf.sample.diff that explains Session-Timers config params. Both diff files are based on the following trunk: http://svn.digium.com/svn/asterisk/team/group/sip_session_timers By: Ronald Chan (loloski) 2007-10-26 20:45:03 After 2 weeks of testing this on my home * machine with a couple of asterisk peer via dundi what i can say is that it's work for me!, if you need us to post sip debug or anything debugging information to prove it's working just ask. By: Olle Johansson (oej) 2007-11-05 08:35:03.000-0600 Ok, I'm taking this on and will submit it soon. /O By: Raj Jain (rjain) 2007-11-05 08:53:07.000-0600 oej: Thanks a lot for this. Please merge chan_sip.c.diff and sip.conf.sample.diff in to the sip_session_timers trunk. chan_sip.c.diff resolves a minor bug and sip.conf.sample.diff describes how the session-timers feature is configured in sip.conf. By: Olle Johansson (oej) 2007-11-05 14:17:58.000-0600 Ok, updated the branch with the latest patches. Changed formatting for CLI commands and added information on manager command too. By: Olle Johansson (oej) 2007-11-06 09:05:32.000-0600 I wont accept function names as "proc_422_rsp" without any explanatory comments. I also get these compilation errors on the branch that needs to be fixed. chan_sip.c: In function 'handle_response_invite': chan_sip.c:13644: warning: implicit declaration of function 'ast_string_field_free' chan_sip.c:13644: error: 'theirtag' undeclared (first use in this function) chan_sip.c:13644: error: (Each undeclared identifier is reported only once chan_sip.c:13644: error: for each function it appears in.) Not sure if "ast_string_field_free" is the right thing to call here. By: Russell Bryant (russell) 2007-11-06 11:07:54.000-0600 I fixed the build issue. By: Raj Jain (rjain) 2007-11-06 12:10:21.000-0600 russell, Thanks for fixing the build issue. oej, I'm uploading a diff file that has some comments on what the proc_422_rsp() function does. We could make this logic inline in the handle_response_invite() similar to how the logic for handling other responses is all inline in handle_response_invite(). My only goal behind creating proc_422_rsp() was to not clutter up handle_response_invite() any further. We can change the name of the function to anything we want or make it inline. Regarding the call to ast_string_field_set() below case 422:, I was trying to model 422 processing somewhat like the way we currently process 401/407 based on their similarities. Both 422 and 401/407 result into generation of a new INVITE. The 401/407 case also calls the ast_string_field_set() function. By: Olle Johansson (oej) 2007-11-15 05:50:58.000-0600 There are a number of undocumented functions here... I only mentioned the first one I saw... By: Olle Johansson (oej) 2007-11-15 05:52:50.000-0600 It's not very good for realtime to run find_peer/find_user every time we need a session timer value. We need to find another solution. By: Olle Johansson (oej) 2007-11-15 05:57:44.000-0600 Updated the branch. - Still need more comments - Merged two functions (se_get_max/min) There are too many calls to find_user and find_peer, we need to get rid of them and cache all these values in a structure in the PVT after first match with a peer or a user, otherwise systems using realtime  will be overloaded. By: Olle Johansson (oej) 2007-11-18 11:53:23.000-0600 Any feedback? By: Raj Jain (rjain) 2007-11-18 11:57:35.000-0600 oej, Thanks for the suggestion regarding caching of user/peer level config. I'm working on the changes you suggested. I should be able to post function comments and caching for user/peer level config in a couple of days. By: Olle Johansson (oej) 2007-11-18 15:31:40.000-0600 Ok, if you have any questions, you know my mail address. I'll be standby :-) /Olle By: Raj Jain (rjain) 2007-11-28 21:24:05.000-0600 All, The latest and greatest sip_session_timers branch crashes for me when processing an INVITE or OPTIONS. I've tried this on a couple of different machines with clean builds etc. but no success. The same configuration works fine for me for 1.4.13. I'm wondering if someone else could try the current sip_session_timers branch and report their results. The crash in my case happens at the following line, which seems completely unrelated to the session-timers patch: #0  ast_hashtab_start_traversal (tab=0x0) at hashtab.c:628 628             if (tab->do_locking) (gdb) where #0  ast_hashtab_start_traversal (tab=0x0) at hashtab.c:628 #1  0x080d4a62 in pbx_find_extension (chan=0x0, bypass=0x0, q=0xb7a069d0, context=0x92baa82 "default", exten=0x2b7f19 "s", priority=1, label=0x0,    callerid=0xb7a06d55 "1000", action=E_MATCH) at pbx.c:1138 #2  0x080da7d2 in pbx_extension_helper (c=0x0, con=0x0, context=0x92baa82 "default", exten=0x2b7f19 "s", priority=1, label=0x0,    callerid=0xb7a06d55 "1000", action=E_MATCH, found=0x0, combined_find_spawn=0) at pbx.c:2465 #3  0x080db8a6 in ast_exists_extension (c=0x92bac10, context=0x11 "", exten=0x11 "", priority=17, callerid=0x11 "") at pbx.c:2984 #4  0x0026fe96 in get_destination (p=0x92b9430, oreq=0xb7a0c0a0) at /usr/src/sip_session_timers/include/asterisk/strings.h:39 ASTERISK-1  0x002b2537 in handle_incoming (p=0x92b9430, req=0xb7a0c0a0, sin=0xb7a0c090, recount=0x92bac10, nounlock=0xb7a0bf74) at chan_sip.c:14846 ASTERISK-2  0x002b4ffa in sipsock_read (id=0x926b6e0, fd=17, events=1, ignore=0x0) at chan_sip.c:16980 ASTERISK-3  0x080b6cde in ast_io_wait (ioc=0x9262730, howlong=1000) at io.c:293 ASTERISK-4  0x002b635c in do_monitor (data=0x0) at chan_sip.c:17208 ASTERISK-5  0x08113ec5 in dummy_start (data=0x11) at utils.c:858 ASTERISK-6 0x00cc5371 in start_thread () from /lib/tls/libpthread.so.0 ASTERISK-7 0x00b2cffe in clone () from /lib/tls/libc.so.6 (gdb) By: Russell Bryant (russell) 2007-11-28 22:02:19.000-0600 Hey murf, the last backtrace posted here is related to your latest pbx core changes.  It may be something you have already fixed in trunk, though. By: Russell Bryant (russell) 2007-11-28 22:04:22.000-0600 rjain, is the version of the sip session timers branch at least a couple of days old?  If so, it was a couple of weeks behind due to the automerge system not running for a couple of weeks on our svn server.  However, I got it up to date yesterday. By: Raj Jain (rjain) 2007-11-29 08:21:44.000-0600 russell, Yes my checkout was a couple of days old. I got the latest this morning and now things are working fine. Thanks. By: Raj Jain (rjain) 2007-11-29 09:49:57.000-0600 oej, russell, I've just now uploaded a file called chan_sip.c.cache.diff. This file implements caching of session-timers related configuration data within sip_pvt. In addition, this file fixes a bug that was introduced when oej merged the st_get_min_se() and st_get_max_se() functions into one st_get_se(). The bug was that if the session-minse and session-expires were not configured at either user or peer level then the function was returning global_max_se instead of global_min_se when the function argument 'max' was set to FALSE. Pls. let me know if you have any comments. Thanks. By: Olle Johansson (oej) 2007-12-04 09:21:26.000-0600 Like I said earlier, I would prefer that the values you include in the PVT is embedded in a structure that is only allocated when we need it. That applies to the peer/user structure too. By: Raj Jain (rjain) 2007-12-04 09:44:00.000-0600 oej, I guess you are worried about the increased size of sip_pvt, sip_user and sip_peer due to the introduction of the session-timers feature. The increase in size for these structs are the following: sip_pvt  = 40 bytes (out of this 16 bytes are used for caching config settings) sip_user = 16 bytes sip_peer = 16 bytes You seem to be suggesting that these fields should be dynamic allocated. While it could be done, it has the risks of complicating the code, potential for memory leaks, breaking feature logic, pointer corruption etc. It also has a fundamental drawback that if we were to think about replicating sip_pvt to a separate memory space then dynamically allocated structs in sip_pvt tends to complicate such a feature. The actual number of bytes needed in sip_pvt/sip_user/sip_peer to support the session-timers feature is pretty low. In light of that is it really worth over-engineering this? Or am I missing your point? By: Olle Johansson (oej) 2007-12-04 10:14:29.000-0600 Well, the SIP_PVT is used for many things that are not calls and I don't want to keep expanding it. Everyone that adds something says "look, it's only 40 bytes"... Sorry, but it needs to be encapsulated. There are previous examples on how to make it work properly and clean it up without the risk of memory leaks, so there's plenty of previous experience. No need to worry. By: Digium Subversion (svnbot) 2007-12-11 01:11:08.000-0600 Repository: asterisk Revision: 92266 U   team/group/sip_session_timers/channels/chan_sip.c ------------------------------------------------------------------------ r92266 | oej | 2007-12-11 01:11:06 -0600 (Tue, 11 Dec 2007) | 9 lines Integrating new code. Still needs some changes before it's ready for prime time. And test reports, please! (issue ASTERISK-10249) Reported by: rjain Patches:      chan_sip.c.cache.diff uploaded by rjain (license 226) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=92266 By: Raj Jain (rjain) 2007-12-26 06:57:26.000-0600 oej: Just uploaded chan_sip.memalloc. This file contains the logic for dynamic allocation of session-timers related data in the form of a struct called sip_st_dlg w/in sip_pvt. I've also wrapped around session-timers related fields in sip_user and sip_peer into a struct called sip_st_cfg. I've tested these changes against the test cases documented in the design doc and also against a couple of commercial SIP end-points. I believe I've addressed all the comments/feedback received so far. Hoping that this feature is now ready for inclusion in the trunk. By: Digium Subversion (svnbot) 2007-12-26 12:53:16.000-0600 Repository: asterisk Revision: 94800 U   team/group/sip_session_timers/channels/chan_sip.c ------------------------------------------------------------------------ r94800 | russell | 2007-12-26 12:53:15 -0600 (Wed, 26 Dec 2007) | 6 lines Apply latest patch from bugs.digium.com ... (issue ASTERISK-10249) Patches:      chan_sip.memalloc uploaded by rjain (license 226) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=94800 By: Raj Jain (rjain) 2007-12-27 15:11:28.000-0600 I'm running into a build issue w/ the latest sip_session_timers branch. The linker is complaining 'can not find -lsensors'. This wasn't happening before. Any ideas? By: snuffy (snuffy) 2007-12-27 17:13:36.000-0600 mm rjain I don't have any issues..(rev 950768) Maybe also try compiling without res_snmp, i think that's the only one that uses lm_sensors By: Raj Jain (rjain) 2007-12-28 10:00:40.000-0600 I did some further testing on the latest sip_session_timers branch in a different setup and found that a bug was introduced when we converted from static to dynamic memory allocation. The file chan_sip.memalloc.bugfix resolves the issue. Pls. merge this fix into the sip_session_timers branch. Thank you. By: Digium Subversion (svnbot) 2007-12-28 19:16:45.000-0600 Repository: asterisk Revision: 95289 U   team/group/sip_session_timers/channels/chan_sip.c ------------------------------------------------------------------------ r95289 | russell | 2007-12-28 19:16:44 -0600 (Fri, 28 Dec 2007) | 4 lines (issue ASTERISK-10249) Patches:      chan_sip.memalloc.bugfix uploaded by rjain (license 226) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=95289 By: Raj Jain (rjain) 2008-01-14 18:55:10.000-0600 Bug Marshals, Any thoughts on when we're going to merge this patch into the trunk? By: Russell Bryant (russell) 2008-01-16 15:51:12.000-0600 This has been merged into trunk as of revision 98978.  Thank you!