6
\$\begingroup\$

This implementation is inspired by the POSIX functions sem_wait() and sem_post(). It tries to get rid of busy loops in the code and appears to work after some simple tests that have been done. But code involving concurrency is hard to get right. Hopefully, there are no race conditions and/or deadlocks in it.

Update 1

semInit() was changed a little bit since the last test. The change had a missing semicolon issue causing syntax error which I have just corrected in this update.

#!/bin/bash

##  Filename: semaphore.bash.inc

##  Blocking other processes by running the function as a
##  process to occupy flock semFile.
lkrRun()
{
    {
        flock -x 5
        while :; do
            sleep 30
        done
    } 5>/tmp/semFile
}

semInit()
{
    local semValArgInit="$1"
    { [ -z "$semValArgInit" ] || [ $semValArgInit -lt 0 ]; } && semValArgInit=0
    echo $semValArgInit >/tmp/semVal
    \rm -f /tmp/lkrPid
    touch /tmp/auxFile
    touch /tmp/semFile
}

semWait()
{
    local semValTmp
    while :; do
        flock -x 5
        [ $(cat /tmp/semVal) -eq 0 ] || {
            semValTmp=$(cat /tmp/semVal)
            let semValTmp--
            echo $semValTmp >/tmp/semVal
            flock -u 5
            break
        }
        { [ -f /tmp/lkrPid ] && kill -0 $(cat /tmp/lkrPid); } || {
            lkrRun &
            echo $! >/tmp/lkrPid
        }
        flock -u 5

        ##  Block point to prevent processes from busying at looping.
        ##  It should still work without it but just become
        ##  inefficient.
        flock -x 6

        flock -u 6
    done 5>/tmp/auxFile 6>/tmp/semFile
}

semPost()
{
    local semValTmp
    {
        flock -x 5
        semValTmp=$(cat /tmp/semVal)
        let semValTmp++
        echo $semValTmp >/tmp/semVal

        ##  Kill the whole locker's process group to relinquish
        ##  flock semFile and resume processes blocked on it.
        [ -f /tmp/lkrPid ] && kill -9 -- -$(cat /tmp/lkrPid)

        \rm -f /tmp/lkrPid
    } 5>/tmp/auxFile
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

It's been a bit since you posted this, but better late than never I hope

First off, on my system the various subshells and subprocesses created by and for lkrRun don't seem to actually go in a new process group. This means that the kill in semPost fails as there is no process group with the given ID, which in turn means that if the semaphore is ever completely exhausted, no call to semWait returns ever again, unless someone manually intervenes (to kill the lock process and/or delete semFile). This is not ideal

There's a variety of ways to fix it, but a simple one could be changing the kill command in semPost to target a single process, and then make sure that killing that process is enough to release the lock. A couple ways to do that might be:

# lkrPid file still handled elsewhere
lkrRun1()
{
    exec flock -xF /tmp/semFile sleep infinity
}

# lkrPid file created and deleted by lkrRun function
lkrRun2()
{
    {
        flock -x 5
        sleep infinity &
        echo "$!" > /tmp/lkrPid
        wait
        rm /tmp/lkrPid
    } 5>/tmp/semFile
}

Second, hardcoding /tmp as the location of the semaphore may not be ideal. It means there can only be one semaphore per system - which is less than ideal if multiple programs will be using this semaphore library, or if multiple users want to run the same program for that matter. Having the functions take a directory as an argument (or in an environment variable) is likely better.

On a related note, since /tmp can usually be written to by any user, we'll want to make sure all relevant files and directories are as locked down as we can make them. For one example of how things can go wrong, if there is a program running as root which uses this semaphore library anyone can convince that program to kill any process by putting its PID in /tmp/lkrPid. To try and avoid stuff like that, we could set a restrictive umask (077 is usually a good bet) before creating any of the files, to ensure they're created in a state where other users can't manipulate them

Unless there's a very good reason to use SIGKILL, using a gentler signal like SIGTERM is usually a good idea. Granted, I don't expect that using SIGKILL does much harm in this particular case, since we know exactly what process we're killing and what it does (unless someone's tampered with /tmp/lkrPid, see above), but still

I'm not loving the names of the lock files. semFile is a lock held to signal that there are no permits available, and released after a delay or when a permit becomes available. This is not clear from the name of the file, in my opinion - from the name alone I'd've expected it to do what semVal does, and after learning semValexists I expectede it to do what auxFile does (namely, guard access to semVal). I'm thinking perhaps something like semVal.lock (for auxFile) and semEmpty.lock (for semFile)?

As a matter of personal opinion, I don't find the use of && and || for conditional execution very readable. if blocks are, in my opinion, often much clearer

Finally, there are some unquoted variables and command substitutions that should probably be quoted

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.