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

presence: Always check if a record exists for this dialog before inserting #724

Merged
merged 3 commits into from
Aug 22, 2016

Conversation

phil-lavin
Copy link
Contributor

@phil-lavin phil-lavin commented Jul 22, 2016

The presence implementation is a little dubious, to say the least.
It probably wants re-writing at some stage. However, this fixes a
race condition that could have a number of causes in which the PUA
is unaware of the eTag at the point it sends the PUBLISH.

The mechanism is such that the PUA passes the eTag that should be updated in the database into a main Kamailio process via a header in the PUBLISH. It is made aware of the new eTag by the main Kamailio process in the 200 OK.

In the scenario when the PUA has not received the 200 OK for the Trying PUBLISH before it sends the early PUBLISH, it will send the early PUBLISH with no eTag. This results in both Trying and early being inserted into the database. Only the early is updated, as it is the most recent, meaning the Trying will stick around in the table until it expires.

…rting

- The presence implementation is a little dubious, to say the least.
  It probably wants re-writing at some stage. However, this fixes a
  race condition that could have a number of causes in which the PUA
  is unaware of the eTag at the point it sends the PUBLISH.
@phil-lavin
Copy link
Contributor Author

This may have a memory leak... Going to investigate more so hold off on merging for now. If anyone can spot it, free beer may be on offer.

@phil-lavin
Copy link
Contributor Author

Found the leak with valgrind. Added as a second commit. Going to run in production for a few days and see if memory stays stable.

@miconda
Copy link
Member

miconda commented Jul 29, 2016

Thanks! I will review and merge asap -- just that these days I am traveling with limited available time and the pull request is consistent.

@phil-lavin
Copy link
Contributor Author

Hi Daniel, no problem at all. Whenever you can.

It's been running in production for a few days now, along with #726. We use BLF extremely heavily and no further problems have been seen by customers.

That said, give it another week or so such that we can be sure the memory footprint is unaffected.

@miconda
Copy link
Member

miconda commented Aug 12, 2016

I guess all went ok so far, without issues, given you haven't reported back. I hope to have the time to do the merge these days.

@phil-lavin
Copy link
Contributor Author

Yes, all good here. #726 has been running good too.

@miconda miconda merged commit 3b2f7ce into kamailio:master Aug 22, 2016
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.

None yet

2 participants