Summary:	ASTERISK-22428: [patch] SIP unregister does not fully unregister when using Realtime sip peers and Expires not 0 on 200ok
Reporter:	Ben Smithurst (bensmithurst)	Labels:
Date Opened:	2013-08-29 11:50:09	Date Closed:	2013-09-25 14:32:03
Priority:	Minor	Regression?	Yes
Status:	Closed/Complete	Components:	Channels/chan_sip/Registration
Versions:	1.8.23.0 11.5.0	Frequency of Occurrence	Constant
Related Issues:	must be completed before resolving ASTERISK-22548 [patch] Add SIP un-register tests is caused by ASTERISK-14953 [patch] Autocreated peers not deleted when unregister explicitly, become zombies is related to ASTERISK-12810 [patch] When unregistering a UA, 200 OK response from Asterisk is not SIP compliant is related to ASTERISK-22548 [patch] Add SIP un-register tests
Environment:		Attachments:	( 0) after.pcap ( 1) asterisk-22428-dont-update-unregistered-peer.diff ( 2) asterisk-22428-rt-peer-update-and-expires-header.diff ( 3) before.pcap ( 4) console_after.txt ( 5) console_before.txt ( 6) realtime_unregister.diff
Description:	When a SIP phone unregisters, this does not seem to be fully saved back to the DB when using realtime.  I see that the ipaddr and port fields are cleared, but regseconds and fullcontact are not, so the registration still works.  Asterisk seems to know it is unregistering, as it logs 'Unregistering SIP xxx' on the console. In console_before.txt you can see that after unregister, a call is placed, and the call is routed to the extension, but in console_after.txt it immediately says everyone is busy/congested, as expected. I have also attached before/after PCAPs and the simple patch I have used to solve this.  The problem is present, and the patch works, in both 11.5.0, and the head of branches/11 (SVN-branch-11-r397758).
Comments:	By: Ben Smithurst (bensmithurst) 2013-08-29 11:51:23.881-0500 I need to sign the agreement before I can upload the patch. By: Michael L. Young (elguero) 2013-08-29 14:13:08.754-0500 Ben, thanks for your report.  I can confirm this bug. I am curious to see the patch you have.  I have come up with a patch as well to fix this issue. What is happening is that, we do clear out the information from the database but it is then added back in.  The peer is unregistered, the data is cleared out, but then after unregistering, a call to update the peer is done which puts back some of the information that was just cleared out. I will attach a patch so that we can compare the fixes to this issue. By: Michael L. Young (elguero) 2013-08-29 19:16:01.625-0500 Give this patch, [^asterisk-22428-dont-update-unregistered-peer.diff], a try and please provide feedback on what you think or if this works. I did some testing with it and everything appears to be working properly.  I ended up modifying the first patch I had in order to work properly in the case where RT Caching is turned off. I am still curious to see your patch though and see if we came to the same conclusion as to how to fix it. Thanks By: Ben Smithurst (bensmithurst) 2013-08-30 05:14:12.164-0500 I can confirm this patch works for us too.  I did it a slightly different way, once my licence agreement is approved I'll upload my patch. (Strangely the patch didn't apply cleanly because of whitespace differences, and I had to use {{patch -l}} - not sure if that's just Jira or my browser mangling it or something.) By: Ben Smithurst (bensmithurst) 2013-08-30 08:45:38.567-0500 I've attached the patch I used - [^realtime_unregister.diff] - different approach, but seems to work. One difference is that Michael's patch still puts 'Expires: 120' in the response to unregister, but mine says 'Expires: 0'. By: Michael L. Young (elguero) 2013-09-12 12:01:06.535-0500 The commit for ASTERISK-14953, r315675, introduced a regression which your patch corrects. This line to set pvt->expiry=0,that is in your patch, was originally committed in r162738 for ASTERISK-12810.  Unfortunately, the patch for ASTERISK-14953 took it out. Good catch on your part Ben! By: Michael L. Young (elguero) 2013-09-16 17:27:09.332-0500 I just wanted to put a note here that I was working on some tests for this which I just finished.  There currently are no authenticated SIP register / un-register tests in the Asterisk testsuite for chan_sip (from what I could see).  Upon working on these tests, I am seeing that Asterisk responds differently when there is an un-authenticated un-REGISTER vs an authenticated un-REGISTER.  I want to track that down and see why. By: Michael L. Young (elguero) 2013-09-17 13:23:04.866-0500 Okay, what I found out was my tests (sipp scenarios) had some flaws.  Once that was worked out the tests failed without the patch, as is expected, and Asterisk responded the same way whether it was an authenticated register or not. There seems to be two issues here.  One dealing with Realtime SIP peers having the database record updated (putting back what was just cleared out) after the peers have been unregistered and one dealing with the handling of the Expires header. Therefore, I have updated the patch accordingly to hopefully handle both of these issues.  Please test it, [^asterisk-22428-rt-peer-update-and-expires-header.diff], and report back if you can.  It would be greatly appreciated. Thanks By: Ben Smithurst (bensmithurst) 2013-09-18 08:02:43.170-0500 Works fine for me. Thanks! By: Michael L. Young (elguero) 2013-09-18 14:00:16.355-0500 Thanks Ben for the prompt testing and confirmation.