HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

Using goto to wait on the result of a database query to change

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
resultthewaitquerydatabasegotousingchange

Problem

This is a small personal project where most non-sensitive data is distributed to some globally hosted CDN servers, which store the JSON. Now one problem we've seen during testing is a few deadlocks during spike loading of Ajax resources at the precise time that the cache is being distributed to the cache servers. So I came up with this locking scheme. Basically it will loop until the cache-lock value from one of the slave db servers responds with a value of 1, where 1 represents readable and 0 represents a locked state.

Is this ok to put into production? The environment is a REST server written in Slim Framework. I'm not smart enough to know why goto is considered evil so I would like a different opinion. I'm also curious if I should choose an alternative to sleep(1).

public function wait_lock()
{
    $steps = 0;
    restart:
    if(! $this->is_enabled('cache-lock') ){
        if($steps > 4){
            // the following method is a short cut for throwing an exception.
            $this->e('cache error. wait a few moments and try again. if the problem persists, contact the administrator.');
        }else{
            sleep(1);
            $steps++;
            goto restart;
        }
    }
}

Solution

Besides the unjustified use of goto, I also object to the use of the number 4 for an operation that you try up to 6 times.

Counting loops are usually best written as for loops.

public function wait_lock() {
    for ($tries = 0; $tries is_enabled('cache-lock')); ++$tries) {
        sleep(1);
    }
    if (!$lock) {
        // the following method is a short cut for throwing an exception.
        $this->e('cache error. wait a few moments and try again. if the problem persists, contact the administrator.');
    }
}

Code Snippets

public function wait_lock() {
    for ($tries = 0; $tries < 6 && !($lock = $this->is_enabled('cache-lock')); ++$tries) {
        sleep(1);
    }
    if (!$lock) {
        // the following method is a short cut for throwing an exception.
        $this->e('cache error. wait a few moments and try again. if the problem persists, contact the administrator.');
    }
}

Context

StackExchange Code Review Q#79426, answer score: 10

Revisions (0)

No revisions yet.