[Home]

Summary:ASTERISK-10498: chan_sip can generate several outstanding requests, but ignores responses
Reporter:Frederic LE FOLL (flefoll)Labels:
Date Opened:2007-10-11 02:25:22Date Closed:2007-11-09 10:34:34.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c.1.4.13.outstandreqs-patch
( 1) chan_sip.c.trunk.86150.outstandreqs-patch
Description:chan_sip can generate several outstanding requests within a dialog. The problem then, is that chan_sip will ignore all responses where CSeq is lower than last outgoing CSeq, which will provoke retransmission of all requests, except last one.

This is especially the case when chan_sip handles a REFER request and generates NOTIFY messages towards the REFER requester : one NOTIFY is transmitted per event during the REFER operations, without waiting for response to previous NOTIFY's. As far as I understand, this is "BAD" but not forbidden (see additional information).

The problem is higly related to the speed of answer of SIP devices implied in the REFER operation.

This behaviour comes from a code section in handle_request() [handle_incoming() in SVN trunk, but the problem seems to be the same] after "if (req->method == SIP_RESPONSE)" :

[...] if (p->ocseq && (p->ocseq != seqno)) {
/* ignore means "don't do anything with it" but still have to
  respond appropriately  */
ignore = TRUE;
ast_set_flag(req, SIP_PKT_IGNORE);
ast_set_flag(req, SIP_PKT_IGNORE_RESP);
append_history(p, "Ignore", "Ignoring this retransmit\n");
} [...]

My first idea is to remove this code, in order let handle_response() do its work (just below). I tried and it seems to work, but I'm not 100% sure of side-effects. Any opinions ?


****** ADDITIONAL INFORMATION ******

Extract of RFC 3515 "The Session Initiation Protocol (SIP) Refer Method" :

3.10 Rate of Notifications

  An event refer NOTIFY might be generated each time new knowledge of
  the status of a referenced requests becomes available.  For instance,
  if the REFER was to a SIP INVITE, NOTIFYs might be generated with
  each provisional response and the final response to the INVITE.
  Alternatively, the subscription might only result in two NOTIFY
  requests, the immediate NOTIFY and the NOTIFY carrying the final
  result of the reference.  NOTIFYs to event refer SHOULD NOT be sent
  more frequently than once per second.
Comments:By: Frederic LE FOLL (flefoll) 2007-10-18 08:54:24

I confirm that chan_sip can generate several outstanding requests within a dialog, without waiting for responses, and this causes many retransmits because chan_sip ignores all responses except response to last request (highest CSeq).

Examples :
- chan_sip transmits several NOTIFY requests when handling a REFER request, and these NOTIFY's can be sent very quickly
- even easier to reproduce, try this sequence in extensions.conf
   exten => 123, 1, Answer()
   exten => 123, n, SendText(ABC)
   exten => 123, n, SendText(DEF)
   exten => 123, n, SendText(GHI)
   ...


By: Frederic LE FOLL (flefoll) 2007-10-18 09:00:42

I propose a patch for SVN trunk : simply delete code in handle_incoming() that rejects SIP RESPONSES with a CSeq lower than last outgoing CSeq !

Indeed, I think that if chan_sip CAN generate several outstanding REQUESTS, if MUST accept RESPONSES with CSeq lower than last outgoing CSeq.

We currently have a 1.4.11 patched that way, and we haven't seen any side effect yet.


By: Frederic LE FOLL (flefoll) 2007-11-02 02:36:20

chan_sip.c.trunk.86150.outstandreqs-patch still applies to current Trunk head : revision 88077.
I add chan_sip.c.1.4.13.outstandreqs-patch that applies to Asterisk 1.4.13 and current 1.4 Branch head : revision 87342.

Between 1.4 and Trunk, two ast_set_flag() have disappeared, and the comment has changed, but the problem is the same.
I disagree with the comment in Trunk, when CSeq in response is lower than last outgoing CSeq : "But in this case this is a response already, so we really have nothing to do with this message, and even setting the ignore flag is pointless". Ignoring "late" responses *is* a problem because chan_sip will illegitimately retransmit requests. Without the patch, we did have the problem with NOTIFY messages that follow a REFER request.

Any comments or feedback ?
By: Olle Johansson (oej) 2007-11-05 03:02:43.000-0600

This is related to another bug report. Since we lack a proper transaction engine, we can't handle multiple transactions. This needs to be fixed.
By: atca_pres (atca_pres) 2007-11-05 13:48:44.000-0600

I have updated issue 9567. I think these issues are related.

Please read the comments in issue 9567

Thank you
By: Digium Subversion (svnbot) 2007-11-07 19:09:30.000-0600

Repository: asterisk
Revision: 89097

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r89097 | file | 2007-11-07 19:09:28 -0600 (Wed, 07 Nov 2007) | 8 lines

Add support for allowing one outgoing transaction. This means if a response comes back out of order chan_sip will still handle it. I dream of a chan_sip with real transaction support.
(closes issue ASTERISK-10498)
Reported by: flefoll
(closes issue ASTERISK-10472)
Reported by: ramonpeek
(closes issue ASTERISK-9288)
Reported by: atca_pres

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-11-07 19:12:35.000-0600

Repository: asterisk
Revision: 89098

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r89098 | file | 2007-11-07 19:12:34 -0600 (Wed, 07 Nov 2007) | 16 lines

Merged revisions 89097 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r89097 | file | 2007-11-07 21:11:25 -0400 (Wed, 07 Nov 2007) | 8 lines

Add support for allowing one outgoing transaction. This means if a response comes back out of order chan_sip will still handle it. I dream of a chan_sip with real transaction support.
(closes issue ASTERISK-10498)
Reported by: flefoll
(closes issue ASTERISK-10472)
Reported by: ramonpeek
(closes issue ASTERISK-9288)
Reported by: atca_pres

........

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-11-09 10:34:34.000-0600

Repository: asterisk
Revision: 89131

_U  team/file/t38fun/
U   team/file/t38fun/apps/app_queue.c
U   team/file/t38fun/apps/app_voicemail.c
U   team/file/t38fun/cdr/cdr_tds.c
U   team/file/t38fun/channels/chan_sip.c
U   team/file/t38fun/codecs/codec_zap.c
U   team/file/t38fun/configs/extensions.ael.sample
U   team/file/t38fun/configs/res_odbc.conf.sample
U   team/file/t38fun/doc/valgrind.txt
U   team/file/t38fun/include/asterisk/lock.h
U   team/file/t38fun/main/config.c
U   team/file/t38fun/main/manager.c
U   team/file/t38fun/main/say.c
U   team/file/t38fun/main/srv.c
U   team/file/t38fun/main/tdd.c
U   team/file/t38fun/pbx/pbx_ael.c
U   team/file/t38fun/res/res_jabber.c
U   team/file/t38fun/res/res_musiconhold.c

------------------------------------------------------------------------
r89131 | file | 2007-11-09 10:34:31 -0600 (Fri, 09 Nov 2007) | 168 lines

Merged revisions 89032,89036-89037,89042,89045-89046,89053,89079,89085,89088,89090,89093,89095,89097,89099,89101,89103,89105,89111,89115,89119,89125 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r89032 | file | 2007-11-06 13:08:05 -0400 (Tue, 06 Nov 2007) | 4 lines

Make it so that if a peer is determined to be unreachable using qualify their devicestate will report back unavailable.
(closes issue ASTERISK-10551)
Reported by: pj

........
r89036 | murf | 2007-11-06 13:52:50 -0400 (Tue, 06 Nov 2007) | 1 line

closes issue ASTERISK-8547 - where the [catname](!) and [catname](othercat1,othercat2,...) notation gets dropped across a ConfigUpdate (or any other thing that would cause a config file to be written). While I was at it, I also cleaned up some of the destroy routines to free up comments, which was not being done. Made sure the new struct I introduced is also cleaned up properly at destruction time. My code handles multiple template inclusions. Many thanks to ssokol for his patch, which, while not literally used in the final merge, served as a foundation for the fix.
........
r89037 | russell | 2007-11-06 14:20:07 -0400 (Tue, 06 Nov 2007) | 11 lines

If someone were to delete the files used by an existing MOH class, and then
issue a reload, further use of that class could result in a crash due to
dividing by zero.  This set of changes fixes up some places to prevent this
from happening.

(closes issue ASTERISK-10500)
Reported by: jcomellas
Patches:
     res_musiconhold_division_by_zero.patch uploaded by jcomellas (license 282)
 Additional changes added by me.

........
r89042 | oej | 2007-11-06 14:53:37 -0400 (Tue, 06 Nov 2007) | 2 lines

Bug fixes to tdd support in zaptel.

........
r89045 | tilghman | 2007-11-06 15:09:06 -0400 (Tue, 06 Nov 2007) | 2 lines

We went to the trouble of creating a method of tracking failed trylocks, then never turned it on (oops).

........
r89046 | qwell | 2007-11-06 15:09:30 -0400 (Tue, 06 Nov 2007) | 4 lines

Correctly set the total number of channels from a zaptel transcoder board.

SPD-49, patch by Matthew Nicholson.

........
r89053 | russell | 2007-11-06 16:18:49 -0400 (Tue, 06 Nov 2007) | 3 lines

Fix init_classes() so that classes that actually do have files loaded aren't
treated as empty, and immediately destroyed ...

........
r89079 | tilghman | 2007-11-07 00:07:49 -0400 (Wed, 07 Nov 2007) | 5 lines

Suppress AEL warnings on load.
Reported by: eliel
Patch by: eliel
Closes issue ASTERISK-10703

........
r89085 | mmichelson | 2007-11-07 11:56:49 -0400 (Wed, 07 Nov 2007) | 5 lines

Fixing a segfault in the manager "core show channels concise" command.

(closes issue ASTERISK-10708, reported by arnd and patched by ys)


........
r89088 | murf | 2007-11-07 17:40:28 -0400 (Wed, 07 Nov 2007) | 1 line

In response to 10578, I just ran 1.4 thru valgrind; some of the config leakage I've already fixed, but it doesn't hurt to double check. I found and fixed leaks in res_jabber, cdr_tds, pbx_ael. Nothing major, tho.
........
r89090 | mmichelson | 2007-11-07 18:40:35 -0400 (Wed, 07 Nov 2007) | 6 lines

This patch makes it possible for SIP phones to dial extensions defined with '#' characters
in extensions.conf AND maintain their escaped characters when forming URI's

(closes issue ASTERISK-10265, reported by cahen, patched by me, code review by file)


........
r89093 | tilghman | 2007-11-07 19:39:37 -0400 (Wed, 07 Nov 2007) | 7 lines

The member refcount must be incremented, to avoid using it after deallocation.
A huge thanks go to lvl- for patiently providing the necessary valgrind output
that was necessary to finding this problem of memory corruption.
Reported by: lvl-
Patch by: tilghman
Closes issue ASTERISK-10699

........
r89095 | file | 2007-11-07 19:53:25 -0400 (Wed, 07 Nov 2007) | 4 lines

If callerid is configured in sip.conf use that for checking the presence of an extension in the dialplan.
(closes issue ASTERISK-10710)
Reported by: spditner

........
r89097 | file | 2007-11-07 21:11:25 -0400 (Wed, 07 Nov 2007) | 8 lines

Add support for allowing one outgoing transaction. This means if a response comes back out of order chan_sip will still handle it. I dream of a chan_sip with real transaction support.
(closes issue ASTERISK-10498)
Reported by: flefoll
(closes issue ASTERISK-10472)
Reported by: ramonpeek
(closes issue ASTERISK-9288)
Reported by: atca_pres

........
r89099 | file | 2007-11-07 21:28:56 -0400 (Wed, 07 Nov 2007) | 6 lines

Improve the devicestate logic for multiple devices. If any are available then the extension is considered available.
(closes issue ASTERISK-9843)
Reported by: nic_bellamy
Patches:
     sip-hinting-svn-branch-1.4.patch uploaded by nic (license 299)

........
r89101 | file | 2007-11-07 22:26:48 -0400 (Wed, 07 Nov 2007) | 4 lines

Do not add a sip: to the beginning of the To URI unless needed.
(closes issue ASTERISK-10331)
Reported by: goestelecom

........
r89103 | tilghman | 2007-11-08 00:55:19 -0400 (Thu, 08 Nov 2007) | 2 lines

Typo

........
r89105 | kpfleming | 2007-11-08 01:26:47 -0400 (Thu, 08 Nov 2007) | 2 lines

fix a glaring bug in the new SRV record handling that would cause incorrect weight sorting

........
r89111 | mmichelson | 2007-11-08 12:47:23 -0400 (Thu, 08 Nov 2007) | 5 lines

I made this same adjustment in trunk to fix a bug, and it makes sense to do it in 1.4 as
well. If an imapfolder is specified in voicemail.conf, don't ever explicitly connect to
INBOX since it may not exist.


........
r89115 | qwell | 2007-11-08 14:45:15 -0400 (Thu, 08 Nov 2007) | 4 lines

Avoid warnings on load when using sample configuration files.

Issue 11195, patch by eliel.

........
r89119 | mmichelson | 2007-11-08 17:00:08 -0400 (Thu, 08 Nov 2007) | 7 lines

Rework of the commit I made yesterday to use the already built-in
ast_uri_decode function as opposed to my home-rolled one. Also added
comments.

Thanks to oej for pointing me in the right direction


........
r89125 | qwell | 2007-11-08 19:52:35 -0400 (Thu, 08 Nov 2007) | 4 lines

Properly say the seconds here..

Issue 11203, fix described by vma.

........

------------------------------------------------------------------------