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

I don't know about PHP, but couldn't you do something like while ($steps > 4) { sleep(1); $steps++; }, then do your error message after the while loop? — user34073
– user34073, Commented Feb 3, 2015 at 5:13
Is it possible to handle this on the cache server side? Perhaps old data could be served until the cache is done switching over so the switch over could seem atomic from the outside. Sleeping in a loop and waiting for the cache to be good seems like fixing the wrong problem. — Corbin
– Corbin, Commented Feb 3, 2015 at 5:54
Corbin, it doesn't make sense to keep old copies of cache, since the row sizes could grow to the millions. its really not practical to keep old copies of the cache. we might experiment with flushing the old cache to a temporary cache server, and serving cache from that during the write lock, so thanks for the idea. — r3wt
– r3wt, Commented Feb 4, 2015 at 1:23

200_success · Accepted Answer · 2015-02-06 03:03:18Z

10

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

edited Feb 6, 2015 at 3:03

answered Feb 3, 2015 at 5:18

200_success

146k22 gold badges191 silver badges481 bronze badges

\$\begingroup\$ overall right, but I still don't like to hide a "big function" (something that connects to a DB or does other important stuff) inside a for-loop condition. If I get an DB-Error, it makes it harder to spot the line with the db-call and harder to debug/set breakpoints in that line. I like to put every statement that does work / has side-effects in its own line - your rev 2 seems a bit too "clever" \$\endgroup\$

Falco
– Falco

2015-02-03 08:58:14 +00:00
Commented Feb 3, 2015 at 8:58
\$\begingroup\$ @Falco I know it's not ideal. Rev 1 had a superfluous sleep(1) after exhausting the last try, so I had to restructure the loop. \$\endgroup\$

200_success
– 200_success

2015-02-03 09:05:07 +00:00
Commented Feb 3, 2015 at 9:05
\$\begingroup\$ I tried to tackle these problems in my answer, but now it looks a little too bloated... \$\endgroup\$

Falco
– Falco

2015-02-03 09:16:58 +00:00
Commented Feb 3, 2015 at 9:16

Add a comment |

Community · Accepted Answer · 2017-04-13 12:40:49Z

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).

my class is a DI Container for a handful of other classes that all rely on that function for returning an exception back to the route scope. throwing an exception via throw results in the error being swallowed, since these calls occur inside of a callback function. other than that, i agree that your solution is more readable, and probably better. — r3wt
– r3wt, Commented Feb 3, 2015 at 5:47
Both revs 1 and 2 try 6 times: first for step = 0 and last for step = 5 (because you try in the while condition - and check the error-condition afterwards, you try one too many times...) — Falco
– Falco, Commented Feb 3, 2015 at 8:55

Falco · Accepted Answer · 2015-02-03 09:14:00Z

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

Stack Exchange Network

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

3 Answers 3

You must log in to answer this question.

Hot Network Questions

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

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions