[Israel.pm] critical section

[Shlomi Fish]

Hi Chanan,

Let me comment on your code and answer your questions.

On Tuesday 19 October 2010 15:18:17 Chanan Berler wrote:
> Hello to all perl programmers
> I am trying to lock some code as critical section:
> my code excepts mode_lock, mutex (filename to lock), and timeout and
> will lock / unlock the file.
> I am running this on RedHOT linux and Perl version 5.10.
> 
> Q1: calling this function to lock the file, will I lose the LOCK_FILE
> due to scope issue ?

No you won't because you have localised it. But it should be a lexical 
variable with a limited scope, and possibly a slot of an object.

> Q2: what will happen if I open a critical section, within another
> critical section ? with diffrent mutex files, will it still work for
> me ?
>       (means will it release the right LOCK_FILE handler ?

Well, there could be a problem if process A locks lock L1 and process B locks 
lock L2 and then A tries to lock L2 and B tries to lock L1, which results in a 
deadlock. There are ways to overcome it.

> Q3: is there a better way to lock / unlock critical sections ?

Maybe look in CPAN. It depends on the constraints of your program.

> thanks
> Chanan
> 

Now for your code.

> 
> sub _get_lock_priority_

Why do you have a trailing underscore in the function name?

> {
>    my ($server, $params) = @_;

There should be an empty line after that.

>    my $succeded_operation = 0;
>    my $mutex_filename = $params->{mutex} . ".lock";
> 
>    # enable critical section
>    if ($params->{mode_lock} eq "GET_LOCK_MODE_ID")
>    {
> 
>        open (LOCK_FILE, "> $mutex_filename");

1. Use three args open.

2. Don't use typeglobs as file handles - use lexicals.

3. Handle an error return value (using "or die or something").

>        foreach (0 .. ($params->{timeout} - 1))
>        {
>             $succeded_operation = 1 if flock (LOCK_FILE , 2|4);

You have magical numbers here - you should use the name LOCK_UN/LOCK_EX/etc. 
constants mentioned in perldoc -f flock.

>             last if ($succeded_operation);

Why not write it as:

if (flock(LOCK_FILE, LOCK_EX()|LOCK_NB()))
{
	$succeeded_operation = 1;
	last TIMEOUT_LOOP;
}

(And make sure you label the loop with TIMEOUT_LOOP;

>             sleep(1);
>        }

>    } else

Don't cuddle your else's. And this is indicative that these should be two 
different subroutines.

>    {
>       # disable critical section
>       $succeded_operation close(LOCK_FILE);

This statement won't compile.

>       foreach (0 .. ($params->{timeout}-1))
>       {
>            if (-e $mutex_filename)
>            {
>                $succeded_operation = 1;
>                last;
>            }
>            sleep(1);      }
>    }
>    return $succeded_operation;

I don't think that closing the lock file will delete this file, so this 
statement will just loop for $params->{timeout} seconds. 

Better just call flock with LOCK_UN().

Regards,

	Shlomi Fish

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Original Riddles - http://www.shlomifish.org/puzzles/

<rindolf> She's a hot chick. But she smokes.
<go|dfish> She can smoke as long as she's smokin'.

Please reply to list if it's a mailing list post - http://shlom.in/reply .