[Home]

Summary:ASTERISK-13923: [patch] lock is not released on channel masquerade
Reporter:Atis Lezdins (atis)Labels:
Date Opened:2009-04-08 10:39:44Date Closed:2009-09-29 23:44:44
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Functions/func_lock
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090717__issue14859.diff.txt
( 1) 20090821__issue14859.diff.txt
( 2) 20090902__issue14859__1.6.1.diff.txt
( 3) 20090925__issue14859__1.6.1.diff.txt
( 4) bt.asterisk-dev-mc-2009-09-25T11:29:33-0700.1304.txt
( 5) func_lock_bt1.txt
( 6) func_lock_masquerade.patch
( 7) lock_debug.log
Description:Scenario:

Queue -> Local channel -> answer_macro -> TRYLOCK -> bridge (masquarade) -> TRYLOCK

First TRYLOCK creates lock correctly, but unlock is called outside of the same thread, so it gives permission denied. Respectively lock remains even if channel is dead or masqueraded, and second attempt to TRYLOCK gives failure.



****** ADDITIONAL INFORMATION ******

Using backport in 1.4.19, but it's probably acting the same on 1.6 tree.

Comments:By: Atis Lezdins (atis) 2009-04-08 13:54:11

Attached idea how to solve this. However I'm uncertain what else this could influence.
By: Atis Lezdins (atis) 2009-05-11 08:14:53

After having a thought of this, perhaps a better way to do this would be - put a datastore on both bridged channels, and unlock as soon as any of them gets destroyed.
By: Tilghman Lesher (tilghman) 2009-07-17 16:44:01

Patch against trunk.
By: Leif Madsen (lmadsen) 2009-07-20 09:51:31

Just a quick question:

What do you mean by:

TRYLOCK -> bridge (masquerade) -> TRYLOCK


What does the dialplan look like to reproduce this?
By: Leif Madsen (lmadsen) 2009-07-20 10:50:24

Feel free to reassign this back to me when there is a valid scenario for reproducing this issue.

Or even better, if the reporter can test in their environment, that would be the ideal.
By: Tilghman Lesher (tilghman) 2009-08-09 02:16:55

atis:  a repeatable case or testing on your part would be most helpful.
By: Atis Lezdins (atis) 2009-08-11 15:01:23

Ok, sorry for the delay, i'm going to test it this week.

lmadsen, what i meant is that first TRYLOCK is called within answer macro of Dial, then call is bridged (masquerade from queue/local channel) and then transfered. So, in the attached log you can see errors before call starts on transfer context and within transfer context lock cannot be released.
By: Tilghman Lesher (tilghman) 2009-08-11 17:31:47

atis:  calling TRYLOCK in the answer macro would be why the lock cannot be released.  That macro runs on the receiving channel, and the macro is the only time that something runs on that channel.
By: Atis Lezdins (atis) 2009-08-11 19:54:37

Tilghman, that's exactly my point. what I'm trying to achieve is not allow several instances of answer macro to execute simultaneously (another hard issue around here)

It is not invalid to use it there, just current behavior is buggy. The lock should either be moved to other channel upon masquerade - so that it can be destroyed in correct thread (as I've done in my patch), or it should be set on both channels and destroyed if one of them dies.

I haven't fully read Your patch and don't yet have idea how You are solving it, but I can test it.

P.S. It's hard to remember issue after several months :)
By: Atis Lezdins (atis) 2009-08-18 06:45:21

Tilghman, Your patch seems to work. I see two errors when calling TRYLOCK, but they don't seem to affect anything:

[2009-08-18 03:04:57.916] ERROR[13961] astobj2.c: bad magic number 0xdeadbeef for 0xb7b70610
[2009-08-18 03:04:57.916] ERROR[13961] astobj2.c: bad magic number 0xdeadbeef for 0xb7b70610


By: Tilghman Lesher (tilghman) 2009-08-18 11:48:57

atis: are you testing this in trunk or in 1.4?  This code is invalid for 1.4.
By: Atis Lezdins (atis) 2009-08-18 15:46:50

This was tested on 1.6.1.4


By: Tilghman Lesher (tilghman) 2009-08-18 16:04:57

It's still invalid on 1.6.1.4.  This changed code is only valid on trunk.
By: Atis Lezdins (atis) 2009-08-18 16:10:21

Ok, I'll test on trunk tomorrow.

However i wonder that it's actually working for 1.6.1 (and repeated locks are  blocked).

As for 1.6.0-1.6.2, perhaps i could implement my initial idea and add datastores to both channels.
By: Atis Lezdins (atis) 2009-08-20 13:09:17

Latest patch seems to work on trunk.

However it remains unclear from logs where the lock is actually destroyed.

For example, if calling TRYLOCK in first local channel, it would say:

func_lock.c: Channel SIP/90099-09439398 has no lock datastore, so we're allocating one.

Then, while that channel is active, if another channel calls TRYLOCK with the same name, it returns 0, but still says that it has no datastore.

Once both local channels are dead, lock seems to be gone, calling subsequential UNLOCK's and TRYLOCK's works as expected.

P.S. i wonder why those locks don't show up in "core show locks", is this intended or that's broken in trunk.
By: Tilghman Lesher (tilghman) 2009-08-20 13:38:43

They don't show up in "core show locks", because they're not implemented as mutexes anymore.
By: Tilghman Lesher (tilghman) 2009-09-01 19:16:21

I've made a few more changes to this patch, since you tested, in order to better ensure that some possible race conditions were solved.  Would you mind testing this again (against trunk)?  There's a somewhat simple change I can make to backport this to 1.6.1, but I want to ensure that this code works well, first.
By: Atis Lezdins (atis) 2009-09-01 19:23:20

actually we're just starting a month full of tests for 1.6.1, so if you have backport it could get tested really good.

i will do a simple works/doesn't work test tomorrow.
By: Tilghman Lesher (tilghman) 2009-09-02 13:49:16

Done.
By: Atis Lezdins (atis) 2009-09-02 14:04:37

Thanks. I can confirm that initial tests for trunk are working as expected.

I'll let You know results of backport tests with much more call volume in a week.
By: Tilghman Lesher (tilghman) 2009-09-20 09:56:23

It has been nearly 3 weeks.  Do you have results of that backport test yet?
By: Atis Lezdins (atis) 2009-09-20 12:18:54

So far we haven't had a single crash while testing functionality.

We'll do some load testing in middle of next week, so I'll report results in a week.
By: Tilghman Lesher (tilghman) 2009-09-25 12:54:19

Aha.  In the 1.6.1 version only, I didn't completely change the linked object to be a NULL object.  The container was still expecting it to be an ast_channel *.
By: Atis Lezdins (atis) 2009-09-25 14:00:11

Latest patch still crashes 1.6.1
By: Tilghman Lesher (tilghman) 2009-09-25 15:23:18

Huh.  I can't get it to crash at all.  Are you running the latest 1.6.1 code from SVN?
By: Digium Subversion (svnbot) 2009-09-29 23:35:20

Repository: asterisk
Revision: 221044

U   trunk/funcs/func_lock.c

------------------------------------------------------------------------
r221044 | tilghman | 2009-09-29 23:35:20 -0500 (Tue, 29 Sep 2009) | 8 lines

Allow locks to be inherited through a masquerade without causing starvation.
(closes issue ASTERISK-13923)
Reported by: atis
Patches:
      20090821__issue14859.diff.txt uploaded by tilghman (license 14)
      20090925__issue14859__1.6.1.diff.txt uploaded by tilghman (license 14)
Tested by: atis, tilghman

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=221044
By: Digium Subversion (svnbot) 2009-09-29 23:37:29

Repository: asterisk
Revision: 221045

_U  branches/1.6.0/
U   branches/1.6.0/funcs/func_lock.c

------------------------------------------------------------------------
r221045 | tilghman | 2009-09-29 23:37:28 -0500 (Tue, 29 Sep 2009) | 15 lines

Recorded merge of revisions 221044 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r221044 | tilghman | 2009-09-29 23:32:36 -0500 (Tue, 29 Sep 2009) | 8 lines
 
 Allow locks to be inherited through a masquerade without causing starvation.
 (closes issue ASTERISK-13923)
  Reported by: atis
  Patches:
        20090821__issue14859.diff.txt uploaded by tilghman (license 14)
        20090925__issue14859__1.6.1.diff.txt uploaded by tilghman (license 14)
  Tested by: atis, tilghman
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=221045
By: Digium Subversion (svnbot) 2009-09-29 23:44:36

Repository: asterisk
Revision: 221046

_U  branches/1.6.1/
U   branches/1.6.1/funcs/func_lock.c

------------------------------------------------------------------------
r221046 | tilghman | 2009-09-29 23:44:36 -0500 (Tue, 29 Sep 2009) | 15 lines

Recorded merge of revisions 221044 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r221044 | tilghman | 2009-09-29 23:32:36 -0500 (Tue, 29 Sep 2009) | 8 lines
 
 Allow locks to be inherited through a masquerade without causing starvation.
 (closes issue ASTERISK-13923)
  Reported by: atis
  Patches:
        20090821__issue14859.diff.txt uploaded by tilghman (license 14)
        20090925__issue14859__1.6.1.diff.txt uploaded by tilghman (license 14)
  Tested by: atis, tilghman
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=221046
By: Digium Subversion (svnbot) 2009-09-29 23:44:43

Repository: asterisk
Revision: 221047

_U  branches/1.6.2/
U   branches/1.6.2/funcs/func_lock.c

------------------------------------------------------------------------
r221047 | tilghman | 2009-09-29 23:44:42 -0500 (Tue, 29 Sep 2009) | 15 lines

Recorded merge of revisions 221044 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r221044 | tilghman | 2009-09-29 23:32:36 -0500 (Tue, 29 Sep 2009) | 8 lines
 
 Allow locks to be inherited through a masquerade without causing starvation.
 (closes issue ASTERISK-13923)
  Reported by: atis
  Patches:
        20090821__issue14859.diff.txt uploaded by tilghman (license 14)
        20090925__issue14859__1.6.1.diff.txt uploaded by tilghman (license 14)
  Tested by: atis, tilghman
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=221047