Eyepea / aiosip

SIP support for AsyncIO (DEPRECATED)
Apache License 2.0
82 stars 41 forks source link

Redo dialog tracking #94

Closed vodik closed 6 years ago

vodik commented 6 years ago

This PR reworks how dialogs are tracked

Its actually wrong to try and first determine the remote address first, and then use that to work out which dialog a message belongs to - we don't have enough information to accurately make that determination.

All we have is sufficient information to determine which dialog and transaction a message belongs to.

Also in the case that a new inbound message arrives and needs to be run through the dialplan, the Contact header should be used instead of the socket's peername for figuring out where to return traffic.

So the current design needs to be broken and globally track all dialogs. Once we've established the dialog, we can get peer information. Dialogs are also now tracked by the set of the address contained in the To and From headers plus the Call-ID. This is much closer to correct, but still a little lacking.

This isn't quite correct, and needs to shift to using just the tags from the To and From header, but that requires some deeper changes as we don't know this information until the response has returned.

Unfortunately this completely breaks a lot of the statistics stored on the peers, and I've removed them for the time being.

Thanks to @moises-silva for the help with this.

vodik commented 6 years ago

I'm going to punt the dialog cleanup work to a separate PR. I think I can clean up a lot of the teardown code in one shot.

vodik commented 6 years ago

Fixes #93. Tested my use cases extensively.

vodik commented 6 years ago

Okay, this PR actually requires another change to happen as well - need to return SIP packets to the Via header, not the remote connection's address information.

This is what broke SIP proxying.

codecov-io commented 6 years ago

Codecov Report

Merging #94 into master will decrease coverage by 0.21%. The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   70.63%   70.42%   -0.22%     
==========================================
  Files          14       15       +1     
  Lines        1553     1562       +9     
  Branches      273      267       -6     
==========================================
+ Hits         1097     1100       +3     
- Misses        340      343       +3     
- Partials      116      119       +3
Impacted Files Coverage Δ
aiosip/peers.py 71.04% <100%> (+2.77%) :arrow_up:
aiosip/dialog.py 73.49% <100%> (-0.89%) :arrow_down:
aiosip/contact.py 78.57% <60%> (-7.43%) :arrow_down:
aiosip/via.py 66.66% <66.66%> (ø)
aiosip/application.py 70.58% <93.75%> (+2.27%) :arrow_up:
aiosip/transaction.py 57.93% <0%> (-4.83%) :arrow_down:
aiosip/message.py 77.61% <0%> (-0.96%) :arrow_down:

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 4770c33...d75e997. Read the comment docs.

vodik commented 6 years ago

This is working really well for me in production now - so I'm going to merge it. I think the benefit of having this working correctly (well, very close to correct) outweighs having contact/registered/subscriber stats on the peer.

I've personally never cared or noticed this was there because I've actually always implemented this kind of stuff client side (I think its more fitting as well, as the two major projects I'm overseeing has to do it differently).

I will open some issues to look at restoring this functionality though.