4 Oct
2016
4 Oct
'16
10:35 a.m.
pres_htable_restore(): Remove check of expired presentity entries when
initially filling the hash table to represent the number of expired +
valid entries. Because hash.c::delete_phtable() decrements publ_count on
removal of _every_ expired DB entry.
get_p_notify_body(): Compensate the fix on hash table restore by
checking for the expires time on building the NOTIFY. This also fixes a
problem when a SUBSCRIBE is received for an expired entry before the
cleaner can remove the entry from the DB.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/809
-- Commit Summary --
* Presence: Fix startup inconsistency in presentity hash table
-- File Changes --
M modules/presence/notify.c (17)
M modules/presence/presentity.c (3)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/809.patch
https://github.com/kamailio/kamailio/pull/809.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809
4 Oct
4 Oct
11:03 a.m.
@Chocolatbuddha pushed 1 commit.
29f6ba4 Fix variable name
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/809/files/c672d23795b17a8238595a4…
7 Oct
7 Oct
3:44 p.m.
Can you provide more details about what is the inconsistency? More information about how
the problem you tried to fix is exposed will help to understand properly the patch.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-252242681
10 Oct
10 Oct
2:58 p.m.
The problem we experienced was that the kamailio would sometimes send empty NOTIFYs after
start. This problem was persistent until the kamailio was restarted again.
We found out that this would occur in case the presentity table contained more or equal
expired entries than valid ones.
Lets assume we have only one presentity entry on startup and it is expired. This entry is
ignored when filling the hash table. A PUBLISH generates a new valid entry in the DB and
the hash table _before_ the first cleanup run resulting in an expired and a valid entry in
the DB and a publ_count of 1 for the URI in the hash table. On first run of the cleaner it
removes the expired entry from the DB and reduces the publ_count to 0 which means removing
it from the hash table despite the fact that there is still a valid DB entry.
Since the following PUBLISHes are subsequent ones only the DB is updated but not the hash
table. The effect are hash table misses resulting in the omission of fetching the XML body
from the presentity table for every following lookup.
This patch primarily tries to make the usage of publ_count consistently represent the
number of presentity entries in the DB. And to make up for the short times an expired
entry is in the hash table (after startup and for recently expired ones during runtime)
the entry's expires time is checked on fetching the body for a NOTIFY.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-252598365
3:22 p.m.
Double checking here, to save time digging the code -- are the expired entry in DB still
deleted properly if they are not loaded anymore?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-252603196
6:17 p.m.
The DB was updated correctly and showed no problems whatsoever in our tests.
The problems were only in the hash table.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-252653703
14 Oct
14 Oct
12:54 p.m.
Before merging, maybe other people using presence modules, like @phil-lavin or @lazedo,
can have a look and comment if they find something inconsistent here, because I don't
have the time right now to go in deeper review and analysis of the code for side effects.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-253757754
6:45 p.m.
@Chocolatbuddha please, make this change with one commit. Just squash those two commits in
one and force push to your branch in order to get a pull request with a single commit
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-253839059
7:56 p.m.
@linuxmaniac I think that can be done from the "Merge pull request" button here
-- it has the right arrow down that gives more options one of them is "Squash and
merge" -- never tried, but from your description it looks like what you want.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-253858573
17 Oct
17 Oct
12:18 p.m.
We know there are issues restarting Kamailio with presentity entries in place. The old
entries cause problems until they expire. We think we have narrowed this down to the pua
db_mode - we can't store PUA entries in the database because that logic is broken in
quite a few ways.
That said, I'll give this patch a try and see if it fixes any of our restart issues
and/or introduces strange side effects.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-254155753
2:26 p.m.
Commits squashed.
(Thanks, @linuxmaniac, for the tip. I didn't know you could do that in Git.)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-254181593
28 Oct
28 Oct
1:05 p.m.
@phil-lavin wondering if you got any chance to test this one in order to merge or not.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-256883963
31 Oct
31 Oct
10:02 a.m.
If none else has comments, probably I will merge this patch in the next few days.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-257233821
1 Nov
1 Nov
12:28 p.m.
It's been running here for a while and it has had no side effects. That said, it
doesn't resolve any problems that we are facing.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-257533932
3:55 p.m.
@phil-lavin: thanks for the feedback, I am going to merge this one.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-257572217
3:55 p.m.
Merged #809.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#event-843376386
3 Nov
3 Nov
6:29 p.m.
this commit broke presence for us. we use NSQ + presence, after a phone reboots and we
send a NOTIFY with confirmed state, presence module sends a NOTIFY with terminated state.
please revert this commit. see the log and SIP packet below:
```
Nov 3 09:23:52 /usr/local/sbin/kamailio[26288]: INFO: <script>:
park+6001(a)troubleshoot.testdomain.com|log|payload
{"Call-ID":"park+6001@troubleshoot.testdomain.com","Event-Category":"presence","Event-Name":"update","Event-Package":"dialog","Expires":"1","From":"sip:park+6001@troubleshoot.testdomain.com","From-User":"park+6001","From-Realm":"troubleshoot.testdomain.com","To":"sip:testuser@troubleshoot.testdomain.com","To-User":"testuser","To-Realm":"troubleshoot.testdomain.com","State":"confirmed"}
Nov 3 09:23:52 /usr/local/sbin/kamailio[26288]: INFO: <script>:
park+6001(a)troubleshoot.testdomain.com|log|received dialog update for
sip:park+6001@troubleshoot.testdomain.com state confirmed
Nov 3 09:23:53 /usr/local/sbin/kamailio[26284]: INFO: presence [notify.c:1634]:
send_notify_request(): NOTIFY sip:testuser@troubleshoot.testdomain.com via
sip:testuser@8.1.2.90:25279;transport=tcp on behalf of
sip:park+6001@troubleshoot.testdomain.com for event dialog :
46ed33b5954833366fb4eda1d08f6bb7
```
```
|NOTIFY
sip:testuser@8.1.2.90:25279;transport=tcp SIP/2.0
6.7.8.219:5055 8.1.2.90:2527 |Via: SIP/2.0/TCP
6.7.8.219:5055;branch=z9hG4bKb0d.16516795000000000000000000000000.0
----------+--------- ----------+---------|To:
<sip:testuser@troubleshoot.testdomain.com>;tag=EBF86DE1-F194FBC6
| NOTIFY | |From:
<sip:park+6001@troubleshoot.testdomain.com>;tag=3d72448f6ec66ad72d4fe275f33a4e85-bf81
09:23:53.007063 | --------------------------> | |CSeq: 3 NOTIFY
+0.007681 | 200 OK | |Call-ID:
46ed33b5954833366fb4eda1d08f6bb7
09:23:53.014744 | <-------------------------- | |Content-Length: 285
| | |User-Agent: kamailio
(5.0.0-dev7 (x86_64/linux))
| | |Max-Forwards: 70
| | |Event: dialog
| | |Contact:
<sip:6.7.8.219:5055;transport=tcp>
| | |Subscription-State:
active;expires=3491
| | |Content-Type:
application/dialog-info+xml
| | |
| | |<?xml
version="1.0"?>
| | |<dialog-info
xmlns="urn:ietf:params:xml:ns:dialog-info" version="3"
state="full" entity="sip:park+6001@troubleshoot.testdomain.com">
| | | <dialog
id="615293b33c62dec073e05d9421e9f48b" direction="recipient">
| | |
<state>terminated</state>
| | | </dialog>
| | |</dialog-info>
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258195982
8:50 p.m.
@eschmidbauer - is it affecting the runtime behaviour, because it should have been
targeting only the startup procedure?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258239094
8:53 p.m.
@miconda yes, we are seeing this issue during runtime. not on startup
we reverted the commit and problem went away
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258239715
9:21 p.m.
the problem with this commit is for expired column = 0 which is usually set on
"early" state
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258247287
10:12 p.m.
@lazedo
you can see that we sending expires=1 in our NSQ message and in nsq_pua.c
```C
struct json_object *ExpiresObj = nsq_json_get_object(json_obj, BLF_JSON_EXPIRES);
if (ExpiresObj != NULL) {
expires = json_object_get_int(ExpiresObj);
if (expires > 0)
expires += (int)time(NULL);
}
```
you can see expires will be set to time + 1
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258260016
10:31 p.m.
@eschmidbauer yep, but that's wrong by definition, we had the same problem. try
sending `expires = 3600` , it will work. i'm ok with this commit, it could be improved
if we were able to ignore the `0` which we interpret as `do not expire`
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258264871
11:17 p.m.
it's not entirely clear to me what this PR fixes. What about adding a module parameter
(default = enabled) to enable this code?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258276622
11:29 p.m.
I am fine adding a parameter to control the changes done by the patch -- being it enabled
or disabled by default, it's up to the people here what they prefer more.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/809#issuecomment-258279847
2971
days inactive
3001
days old
23 comments
6 participants
participants (6)
-
Daniel-Constantin Mierla -
Emmanuel Schmidbauer
-
lazedo
-
Phil Lavin
-
Stefan Kohlhauser
-
Victor Seva