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

Question

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;
        }
    }
}

Answer 1

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 < 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.');
    }
}

Answer 2

Why not just say

public function wait_lock()
{
    $try_count = 0;
    while ( ! $this->is_enabled('cache-lock') ) {
        $try_count++;
        if ( $try_count >= 6 ) {
            // 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.');
        }

        sleep(1);
    }
}

That's shorter than your goto version and clearer about what it is doing.

The first argument against goto is that it allows you to do confusing things, e.g. move across scopes. That's not really a problem here.

The second argument is that it can be hard to read. I had to spend some time staring at this to see where the actual work of the function was being done. You want to loop until the 'cache-lock' is enabled, so just do that.

Also, you don't actually need an else there. If you throw an exception, it will stop processing this function and go up to wherever the exception gets caught. The else implies that you'll continue looping in that case.

I'm also not crazy about the helper function for the exception. Do you really do so much in it that it is better than throwing the exception direct? There's a definite loss of readability by having a function called e throwing an exception. You compensate for this with the comment. Wouldn't it be easier to just include the entire exception throwing code there? It's hardly longer than what you have and shorter than the code plus the comment.

And 200_success is right that saying 6 is better than 4 if you are trying 6 times. Congratulations to Falco for noticing that you are actually trying 6 times with $steps > 4.

$steps | Attempts
-------+---------
   0   |    1
   1   |    2
   2   |    3
   3   |    4
   4   |    5
   5   |    6

This is because you check $steps > 4 after checking if the lock is enabled. So simply changing to $steps >= 5 (as I did originally) is not enough. You have to either start with $steps = 1 which is untrue (at the time that you say it, there have been zero attempts) or move the increment (which keeps a correct count).

Originally I didn't bother to change the name of $steps, but I would prefer something singular for a scalar. The name $try_count exactly describes what it is tracking and is robust in the face of changes to the sleep period (as is $steps).

Answer 3

I think for maintainable code you should use idiomatic syntax. If you want to repeat an action multiple times, this calls for a loop. If you know how many times, usually a for-loop. If you want to break repetitions on a certain condition one uses break or return:

public function wait_lock()
{
  for ( $tries = 0; $tries < 5; ++$tries )
  {
    $lock = $this->is_enabled('cache-lock');

    if ( $lock )  // Lock acquired everything is good
    {
      return;
    }
    else if ( $tries+1 == 5 )  // maximum tries reached: ERROR
    {
       // 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  // Wait one second to reduce server-workload
    {
      sleep(1);
    }
  }
}