Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dialog tracking to use tags & call_id #112

Merged
merged 7 commits into from
May 7, 2018
Merged

Update dialog tracking to use tags & call_id #112

merged 7 commits into from
May 7, 2018

Conversation

ovv
Copy link
Contributor

@ovv ovv commented Apr 25, 2018

Added async-timeout in dev requirements
as it's useful for testing & debugging

@ovv
Copy link
Contributor Author

ovv commented Apr 25, 2018

Fix #96

@ovv ovv force-pushed the dialog-tracking branch 2 times, most recently from 5d7bda4 to 7664f86 Compare April 25, 2018 11:30
@ovv ovv requested a review from vodik April 25, 2018 11:34
@codecov-io
Copy link

codecov-io commented Apr 25, 2018

Codecov Report

Merging #112 into master will decrease coverage by 2.14%.
The diff coverage is 71.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   73.62%   71.48%   -2.15%     
==========================================
  Files          15       15              
  Lines        1547     1487      -60     
  Branches      267      256      -11     
==========================================
- Hits         1139     1063      -76     
- Misses        298      316      +18     
+ Partials      110      108       -2
Impacted Files Coverage Δ
aiosip/transaction.py 65.48% <ø> (-0.73%) ⬇️
aiosip/protocol.py 59.22% <33.33%> (-3.16%) ⬇️
aiosip/dialog.py 75.42% <77.77%> (-2.47%) ⬇️
aiosip/peers.py 78.6% <78.57%> (+4.47%) ⬆️
aiosip/application.py 72.15% <87.5%> (-5.13%) ⬇️
aiosip/utils.py 43.58% <0%> (-23.08%) ⬇️
aiosip/contact.py 67.14% <0%> (-11.43%) ⬇️
aiosip/message.py 73.8% <0%> (-3.81%) ⬇️
... and 2 more

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 af98795...2dd0ceb. Read the comment docs.

@ovv ovv force-pushed the dialog-tracking branch 4 times, most recently from 90ce81a to ae520ef Compare April 25, 2018 14:41
ovv added a commit that referenced this pull request Apr 26, 2018
Imrpove application cleanup. This might need another pass
after #112 is merged

Issue #91 #92
@ovv ovv mentioned this pull request Apr 26, 2018
ovv added a commit that referenced this pull request Apr 26, 2018
Imrpove application cleanup. This might need another pass
after #112 is merged

Issue #91 #92
ovv added a commit that referenced this pull request Apr 26, 2018
Imrpove application cleanup. This might need another pass
after #112 is merged

Issue #91 #92
ovv added a commit that referenced this pull request Apr 26, 2018
Imrpove application cleanup. This might need another pass
after #112 is merged

Issue #91 #92
@vodik
Copy link
Contributor

vodik commented Apr 27, 2018

I'm missing something here, what updates the dialog structure with the To tag once its been set?

ovv added a commit that referenced this pull request Apr 27, 2018
Imrpove application cleanup. This might need another pass
after #112 is merged

Issue #91 #92
ovv added 4 commits April 27, 2018 11:38
Commented the proxy test for now as I'm working on
proxying in a separate branch

Added async-timeout in dev requirements
as it's useful for testing & debugging
@ovv ovv force-pushed the dialog-tracking branch from ae520ef to b25a0ab Compare April 27, 2018 09:40
@ovv
Copy link
Contributor Author

ovv commented Apr 27, 2018

Do not merge yet. I'd like to do some more test after the rebase

@ovv
Copy link
Contributor Author

ovv commented Apr 27, 2018

So the tag gets added as part of the inbound processing? Okay, I think that might should be okay. Might be a little surprising if you dump the message as you don't see the original message anymore, but should be okay.

Yeah I thought about that. In the future the original message To and From should be a snapshot of the dialog To / From. That way we can update the details without changing the original message.

@vodik
Copy link
Contributor

vodik commented Apr 27, 2018

I think it might make sense to hide the original message and pull useful information out of the Request object - at least, that was my plan. I'm only half way there, which is why I left that class embedded right inside the dialog logic.

And in that case, it might make it easier to refactor if its turns out we can't put the tag on the message immediately - have it in the Request object instead.

I mean, in theory, we can do SIP forking (see #100) inside a single endpoint - an INVITE comes in and we send two distinct responses each with a different tag. Maybe the best API for this is to be able to fork the request object....

@vodik
Copy link
Contributor

vodik commented Apr 30, 2018

@moises-silva ^ ping if you don't mind.

@ovv ovv force-pushed the dialog-tracking branch from 9d26f8b to fb013ce Compare May 2, 2018 11:10
@ovv ovv force-pushed the dialog-tracking branch from fb013ce to 2dd0ceb Compare May 2, 2018 11:29
@ovv
Copy link
Contributor Author

ovv commented May 2, 2018

Removed some more proxy stuff since it's not working.
I'll look into it in a future PR

@ovv ovv merged commit 8a2a81e into master May 7, 2018
@ovv ovv deleted the dialog-tracking branch May 7, 2018 10:07
ovv referenced this pull request May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants