Andrei Pelinescu-Onciul writes:

...

K's tm module can be configured to have transaction AVPs or message AVPs: http://kamailio.org/docs/modules/1.5.x/tm#id2530480

What is the behaviour of ser's tm module? I need AVPs in reply route - is it supported?

In theory, yes by default. In practice it looks like there's a bug if several replies are processed in the same time. As a quick fix for sr_3.0 I might introduce the same mechanism as in k.

i didn't quite get this. in sr_3.0, i do see avps in onrely_route that were set in route block before t_relay was called. is this what andrei means in above by default behavior? if so that is fine with me.

-- juha

Klaus Darilion writes:

Juha Heinanen wrote:

...

Andrei Pelinescu-Onciul writes:

...

...

K's tm module can be configured to have transaction AVPs or message AVPs: http://kamailio.org/docs/modules/1.5.x/tm#id2530480

What is the behaviour of ser's tm module? I need AVPs in reply route - is it supported?

In theory, yes by default. In practice it looks like there's a bug if several replies are processed in the same time. As a quick fix for sr_3.0 I might introduce the same mechanism as in k.

i didn't quite get this. in sr_3.0, i do see avps in onrely_route that were set in route block before t_relay was called. is this what andrei means in above by default behavior? if so that is fine with me.

Yes. So sr has a different default behavior than k. (should be documented in migration guide)

what is the conclusion about this?

it is ok with me if in in sr_3.0 onreply_avp_mode is on by default (and even on always), but i'm very worried, if it does not work when several replies are processed simultaneously.

-- juha

Andrei Pelinescu-Onciul writes:

I'll have a quick fix in the next few days (similar to k, but I'm thinking of going for read-only avps by default and a modparam to turn them r/w at the price of locking the reply route). In the long run we need a better fix.

andrei,

i'm also setting avps in onreply_route and thus have to set the r/w modparam. will locking be more costly in sr than in k? would it be possible to lock only those parts of onrely_route where avps are set?

-- juha

Andrei Pelinescu-Onciul writes:

What I'm afraid is that executing the reply route under lock might introduce some deadlocks (it is possible that some functions that are now allowed to be executed from the onreply route would cause problems, I haven't checked all of them).

what could those be? i test/set flags/avps/vars and call functions that rewrite some parts of the message, like contact uri and sdp.

The long term solution would be to lock only the avps and only when used, but it requires lots of changes and testing and I'm not sure it would be ready/good enough for 3.0.

not for 3.0, which, in my opinion, we should get out before end of this year.

-- juha

Hello,

Andrei, instead of having K compat mode, as global parameter, I would prefer to have per transaction function:

t_lock_onreply()

Gives more flexibility.

Cheers, Daniel

On 12/10/09 12:16 PM, Andrei Pelinescu-Onciul wrote:

On Dec 10, 2009 at 12:57, Juha Heinanenjh@tutpro.com wrote:

...

Andrei Pelinescu-Onciul writes:

...

What I'm afraid is that executing the reply route under lock might introduce some deadlocks (it is possible that some functions that are now allowed to be executed from the onreply route would cause problems, I haven't checked all of them).

what could those be? i test/set flags/avps/vars and call functions that rewrite some parts of the message, like contact uri and sdp.

tm functions or functions that use tm api and try to lock replies (lock the transaction reply_lock). I don't know of any and I don't think we'll have any problems, but I haven't checked everything. All the k modules functions that worked in k with the reply avp mode will work with sr too so this leaves possible problems only in modules_s. Everything you mentioned above won't cause any problems.

...

...

The long term solution would be to lock only the avps and only when used, but it requires lots of changes and testing and I'm not sure it would be ready/good enough for 3.0.

not for 3.0, which, in my opinion, we should get out before end of this year.

Yes, I agree.

Andrei

---

sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

On Dec 10, 2009 at 12:34, Daniel-Constantin Mierla miconda@gmail.com wrote:

On 12/10/09 12:32 PM, Daniel-Constantin Mierla wrote:

...

Hello,

Andrei, instead of having K compat mode, as global parameter, I would prefer to have per transaction function:

t_lock_onreply()

or have t_on_reply(...) accepting a second parameter that indicates the locking status for onreply_route execution.

I see 3 possibilities of fixing it:

1. make parallel avp operation safe using no locks

This would require changes to the way avps are deleted: instead of immediately unlinking them from the avp list and freeing their memory they would have to be only marked as deleted. The memory reclaiming/real delete will happen only when the transaction is destroyed, or for normal route avps, at the end of the route.

Just to clarify things $a=something involves 2 avp operations: avp_destroy($a) ; add_new_avp($a, something). So each assignment involves and avp delete/destroy. An onreply_route containing something like $avp="foo" will eat memory for each reply. However this shouldn't really be a problem unless someone uses really huge avps.

Another disadvantage is that add_avp() will become slightly slower. OTOH hand this should be offset by the faster destroy_avp.

This change will make the avp safe to use from routes executed in parallel, however there is no guarantee that they will have the expected value (it would be like changing unprotected variables from parallel threads). E.g.: $a=$a+1 in an onreply route will be undefined if the route is executed in parallel (e.g. after 2 parallel replies $a could be 1 or 2). To offset that we could introduce what Daniel proposed (t_lock_onreply(), t_unlock_onreply()).

2. start with an empty avp list and load the avps and lock the onreply_route only if certain commands are used (e.g. t_onreply_load_avps(), t_onreply_unload_avps()) or a global parameter is set (like k but with the addition of the t_onreply*).

3. based on a config param, either allow only read-only access to avps (and no locking) or read-write access (and locking).

Andrei

On Dec 10, 2009 at 18:23, Daniel-Constantin Mierla miconda@gmail.com wrote:

On 12/10/09 5:02 PM, Andrei Pelinescu-Onciul wrote:

...

On Dec 10, 2009 at 12:34, Daniel-Constantin Mierlamiconda@gmail.com wrote:

...

On 12/10/09 12:32 PM, Daniel-Constantin Mierla wrote:

...

Hello,

Andrei, instead of having K compat mode, as global parameter, I would prefer to have per transaction function:

t_lock_onreply()

or have t_on_reply(...) accepting a second parameter that indicates the locking status for onreply_route execution.

I see 3 possibilities of fixing it:

1. make parallel avp operation safe using no locks

This would require changes to the way avps are deleted: instead of immediately unlinking them from the avp list and freeing their memory they would have to be only marked as deleted. The memory reclaiming/real delete will happen only when the transaction is destroyed, or for normal route avps, at the end of the route.

Just to clarify things $a=something involves 2 avp operations: avp_destroy($a) ; add_new_avp($a, something). So each assignment involves and avp delete/destroy. An onreply_route containing something like $avp="foo" will eat memory for each reply. However this shouldn't really be a problem unless someone uses really huge avps.

Another disadvantage is that add_avp() will become slightly slower. OTOH hand this should be offset by the faster destroy_avp.

This change will make the avp safe to use from routes executed in parallel, however there is no guarantee that they will have the expected value (it would be like changing unprotected variables from parallel threads). E.g.: $a=$a+1 in an onreply route will be undefined if the route is executed in parallel (e.g. after 2 parallel replies $a could be 1 or 2). To offset that we could introduce what Daniel proposed (t_lock_onreply(), t_unlock_onreply()).

2. start with an empty avp list and load the avps and lock the

onreply_route only if certain commands are used (e.g. t_onreply_load_avps(), t_onreply_unload_avps()) or a global parameter is set (like k but with the addition of the t_onreply*).

3. based on a config param, either allow only read-only access to avps

(and no locking) or read-write access (and locking).

I think 3. is much easier to implement comparing with 1. and less cfg complex than 2. I would go for it for 3.0 releases, then look for something better for future.

Actually after re-thinking (and speaking with Miklos), I don't think that always executing the onreply_route under lock would affect performance. We do take the reply lock anyway and so the only advantage of executing the reply route out of the lock would be the possibility to run it in parallel for the same transaction, or in parallel with the failure route. OTOH the probability of having 2 replies for the same transaction processed in parallel is very low.

Andrei

On 12/10/09 12:56 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

Andrei, instead of having K compat mode, as global parameter, I would prefer to have per transaction function:

t_lock_onreply()

Gives more flexibility.

daniel,

does this mean that i would need to call t_lock_onreply() as the first thing in onreply_route?

if so, i would prefer a global parameter that defaults to locking that script writer could override if/when needed. otherwise script writing would become too error-prone

I was thinking of being able to control how the reply routes are executed per transaction, not globally. I think my second proposal is better:

t_on_reply("1"); - run onreply_route[1] with no locking (default, same syntax as now) for current transaction. t_on_reply("1", "0"); - run onreply_route[1] with no locking for current transaction. t_on_reply("1", "1"); - run onreply_route[1] under lock for current transaction.

In some cases, there is no need to lock for replies (no avp processing), therefore why to have it locked.

Cheers, Daniel

On 12/10/09 1:08 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

t_on_reply("1"); - run onreply_route[1] with no locking (default, same syntax as now) for current transaction. t_on_reply("1", "0"); - run onreply_route[1] with no locking for current transaction. t_on_reply("1", "1"); - run onreply_route[1] under lock for current transaction.

fine, if t_on_reply("1") is equal to t_on_reply("1", "1"). this is very important for backwards compatibility and error prevention. t_on_reply("1", "0") is then for optimization purpose.

that was a proposal. Maybe others can have different opinions. I need this option in several cases, not that often. Note that in K it is by default off. So in case you do not have set the tm parameter onreply_avp_mode to 1, then it is eqiuvalent of t_on_reply("1", "0").

Cheers, Daniel

On 12/10/09 1:17 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

that was a proposal. Maybe others can have different opinions. I need this option in several cases, not that often. Note that in K it is by default off. So in case you do not have set the tm parameter onreply_avp_mode to 1, then it is eqiuvalent of t_on_reply("1", "0").

in k per transaction onreply avps are off by default.

if i understood correctly, in sr they are always on. default in sr thus needs to be same as if k user turns per transcation onreply avps on, which means that locking is on.

in your previous email you referred to backward compatibility and I assumed you talk about Kamailio. In kamailio it takes two operations: set tm parameter onreply_avp_mode to 1 and arm onreply_route viat t_on_reply(). Default for t_on_reply() is not to set in locking mode.

However, for me is ok either ways, the one that implements (if is going to be this solution) should decide. Furthermore, default mode cand be dependent of config compatibility mode controlled via #!KAMAILIO or #!SER.

Cheers, Daniel

On 12/10/09 1:28 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

in your previous email you referred to backward compatibility and I assumed you talk about Kamailio. In kamailio it takes two operations: set tm parameter onreply_avp_mode to 1 and arm onreply_route viat t_on_reply(). Default for t_on_reply() is not to set in locking mode.

i was referring to compatibility between sr and k when onreply_avp_mode = 1. if i have understood correctly, in sr there is no support for k onreply_avp_mode = 0 and thus no compatibility with that mode.

ok. there are actually two things: 1) what avp list is made available in onreply_route 2) how the onreply_route is executed

In K, for 1), by default an empty AVP list is made available for each reply. If onreply_avp_mode=1, then the transaction avp list is made available in onreply_route and onreply_route is executed under a lock, to avoid races between processing many replies at same time.

In sr, as I understood, by default the transaction AVP list is made available for each reply, but no lock is used when executing the onreply_route.

Cheers, Daniel

On 12/10/09 1:48 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

In K, for 1), by default an empty AVP list is made available for each reply. If onreply_avp_mode=1, then the transaction avp list is made available in onreply_route and onreply_route is executed under a lock, to avoid races between processing many replies at same time.

In sr, as I understood, by default the transaction AVP list is made available for each reply, but no lock is used when executing the onreply_route.

that is how i understood it too. now andrei just needs to add the missing locking and after that there is compatibility with k onreply_avp_mode=1 without any other changes.

locking for all replies is not present by default in any of the previous versions of K or S. That can be noticeable and I wanted to underline is not backward compatible neither with K or S.

Daniel

On 12/10/09 2:05 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

locking for all replies is not present by default in any of the previous versions of K or S. That can be noticeable and I wanted to underline is not backward compatible neither with K or S.

are you saying that in k when onreply_avp_mode=1, not all replies are locked?

:-) -- I was saying, that by default, neither S nor K lock for reply processing. Now you want to make that default behavior.

Cheers, Daniel

On 12/10/09 2:20 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

:-) -- I was saying, that by default, neither S nor K lock for reply processing. Now you want to make that default behavior.

which in sr may lead to crash and needs to be fixed by adding the locking. in k there is not that problem, because if you choose per transaction mode (which is the only mode that sr has), you get locking on by default.

so the issue is fixing the only mode that sr has. if you want another mode, then someone has to add it write the code and add to sr.

and that is done by adding the mode (no matter is going to be per transaction or global). So you get to K style (more or less) and now is the matter of what should be the default -- locking or not for replies -- and I said that none had locking by default so far.

Daniel

On 12/10/09 2:30 PM, Juha Heinanen wrote:

Daniel-Constantin Mierla writes:

...

and that is done by adding the mode (no matter is going to be per transaction or global).

fine if you add that mode. andrei only promised to add locking to the only mode there currently exists.

Then I understood differently the email, which mentions a r/w avp mode with onreply_route locking in the fix:

http://lists.sip-router.org/pipermail/sr-dev/2009-December/005198.html

Afterwards I came up with the idea to have this mode per transaction, not global option -- that was a proposal.

Cheers, Daniel

Andrei Pelinescu-Onciul wrote:

On Dec 04, 2009 at 09:54, Klaus Darilion klaus.mailinglists@pernau.at wrote:

...

Hi!

K's tm module can be configured to have transaction AVPs or message AVPs: http://kamailio.org/docs/modules/1.5.x/tm#id2530480

What is the behaviour of ser's tm module? I need AVPs in reply route - is it supported?

In theory, yes by default.

I tested it and AVPs were available in reply-route. But I tested only a simple scenario without concurrent replies.

In practice it looks like there's a bug if several replies are processed in the same time.

Ups, that's bad :-)

Klaus

As a quick fix for sr_3.0 I might introduce the same mechanism as in k.

Andrei