vodik commented 6 years ago

Putting this up for review to get some feedback and get some ideas on where you guys are going:

Basically, I want to improve the sip timer support while exposing things in such a way that timing mechanisms can be tweak-able.

Opinions, concerns?

codecov-io commented 6 years ago

Codecov Report

Merging #47 into master will decrease coverage by 0.16%. The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   74.48%   74.32%   -0.17%     
==========================================
  Files          15       16       +1     
  Lines        1360     1398      +38     
  Branches      250      262      +12     
==========================================
+ Hits         1013     1039      +26     
- Misses        233      236       +3     
- Partials      114      123       +9

Impacted Files	Coverage Δ
aiosip/transaction.py	`65.29% <100%> (+0.72%)`	:arrow_up:
aiosip/timer.py	`65.11% <65.11%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fc75f04...4cce60b. Read the comment docs.

ovv commented 6 years ago

I'm not sure this would work ok with the QueueTransaction and I'm a bit confused what is the goal (maybe due to my lack of SIP knowledge).

What I understand is that some request use another timeout. In this case would it not be easier to allow to pass some arguments (like timeout, step and max_timeouts) when creating the Transaction? Or am I missing something ?

At the moment I'm looking into enriching Dialplan to have more custom routing and I would also like to add some SIP test to check our messages are correctly formatted. Maybe you have some ideas on how to implement that ?

vodik commented 6 years ago

SIP has a pile of timers (that are supposed to be tunable) to be RFC compliant. The single timer we have in the code base is just timer B/F running double duty

From the RFC:

Timer    Value            Section               Meaning
----------------------------------------------------------------------
T1       500ms default    Section 17.1.1.1     RTT Estimate
T2       4s               Section 17.1.2.2     The maximum retransmit
                                               interval for non-INVITE
                                               requests and INVITE
                                               responses
T4       5s               Section 17.1.2.2     Maximum duration a
                                               message will
                                               remain in the network
Timer A  initially T1     Section 17.1.1.2     INVITE request retransmit
                                               interval, for UDP only
Timer B  64*T1            Section 17.1.1.2     INVITE transaction
                                               timeout timer
Timer C  > 3min           Section 16.6         proxy INVITE transaction
                           bullet 11            timeout
Timer D  > 32s for UDP    Section 17.1.1.2     Wait time for response
         0s for TCP/SCTP                       retransmits
Timer E  initially T1     Section 17.1.2.2     non-INVITE request
                                               retransmit interval,
                                               UDP only
Timer F  64*T1            Section 17.1.2.2     non-INVITE transaction
                                               timeout timer
Timer G  initially T1     Section 17.2.1       INVITE response
                                               retransmit interval
Timer H  64*T1            Section 17.2.1       Wait time for
                                               ACK receipt
Timer I  T4 for UDP       Section 17.2.1       Wait time for
         0s for TCP/SCTP                       ACK retransmits
Timer J  64*T1 for UDP    Section 17.2.2       Wait time for
         0s for TCP/SCTP                       non-INVITE request
                                               retransmits
Timer K  T4 for UDP       Section 17.1.2.2     Wait time for
         0s for TCP/SCTP                       response retransmits

and

   The default value for T1 is 500 ms.  T1 is an estimate of the RTT
   between the client and server transactions.  Elements MAY (though it
   is NOT RECOMMENDED) use smaller values of T1 within closed, private
   networks that do not permit general Internet connection.  T1 MAY be
   chosen larger, and this is RECOMMENDED if it is known in advance
   (such as on high latency access links) that the RTT is larger.

ovv commented 6 years ago

Thanks I got a better understanding now (I should really dig into the RFC at one point).

Your approach seems good. There are lot of different timers for UDP/TCP this might be a bit more tricky to implement with a nice API.

vodik commented 6 years ago

Yeah I agree, but I got something in mind. However I'm going to backburn this for a bit... I think I found an issue with NOTIFY/SUBSCRIBE at work that I need to fix.

vodik commented 6 years ago

Rejigged this PR and I think its in mergable state if you guys are happy with it:

I leave T1/T2/T4 easily tunable: aiosip.timer.T1 = 1.0 # increase RTT time
I leave the other timers tunable, but more difficult: aiosip.timer.timer_b.override(20)
I've renamed all the timers in use in the dialog logic to retransmit_invite to mark that they're all currently pegged to the same timer (as it was before).

I need to go through and figure out which timer belongs to which piece of logic.

ludovic-gasc commented 6 years ago

Your approach is ok for me. @ovv ?

vodik commented 6 years ago

I'm going to close this and bring it in with the work I'm doing around #59

Eyepea / aiosip

[RFC] Start adding general sip timer support - do not merge #47

Codecov Report