4

https://github.com/civicrm/civicrm-core/blob/a2976e7c0c2515379e98ee9b5ebcf51acde4e540/CRM/Mailing/BAO/Mailing.php#L2867

  $lockArray = range(1, $mailerJobsMax);
  shuffle($lockArray);

  // check if we are using global locks
  foreach ($lockArray as $lockID) {
    $cronLock = Civi::lockManager()->acquire("worker.mailing.send.{$lockID}");
    if ($cronLock->isAcquired()) {
      $gotCronLock = TRUE;
      break;
    }
  }

Why do we randomise the order of the array using shuffle()? Is there a good reason for doing this?

  • Good question. And I don't have the answer. May I ask why you are asking this question? – Jaap Jansma - CiviCooP Nov 26 '15 at 15:37
  • If it's there for a reason then great - I'd love to know why. If it's not then I'll get rid of it as part of some other tidying. I'm currently picking apart the Mailing process, trying to debug something, so anything that looks a bit 'weird' is throwing me off ;] – John Nov 26 '15 at 15:49
  • @JaapJansma when you write "good question" don't forget to also up-vote :) – Coleman Dec 1 '15 at 20:28
3

I don't know the original authors' intent but can find some background and share a theory:

  1. Background: To figure out the origin of some code, it sometimes helps to spot-check the branches in the old Subversion repository. This can require a bit of digging (to trace through incidental merges and cleanups in the history), but ultimately the shuffle() originated in r35243 and CRM-8460. Alas, there's no commentary in there about this detail. You could ping the original author.

  2. Theory: Statistically, shuffling reduces the number of lock-acquisition attempts in one typical scenario. Suppose you're starting up a bunch of workers at the same time. (Maybe you're starting 5 workers, but you've set a limit of 10 workers.) Compare what happens with and without random polling order:

    • (Without shuffle()) When worker #2 starts and tries its first attempt at lock-acquisition, there's a 100% chance of failure, so it will have to do a second lock-acquisition attempt. Similarly, when worker #3 starts and tries its first lock-acquisition, there's a 100% chance of failure.
    • (With shuffle()) When worker #2 starts and tries its first lock-acquisition, there's 10% (1/10) chance of failure. Similarly, when worker #3 starts, there's a 20% chance.

Now, even if we accept this theory, it might not amount to much. In a typical Civi deployment, you have a single server with maybe 1-10 workers, and locks are acquired infrequently. The total overhead isn't very high (esp. in comparison to the work being done). However, if one had thousands of workers, or if the communications channels were slow, or if you had to acquire the locks frequently (every second), then it would add up... so it wouldn't be surprising if some folks viewed it as a generally good practice to shuffle when polling.

0

There doesn't seem to be a good reason, so: https://github.com/civicrm/civicrm-core/pull/7338

Tim's answer demonstrates that there is a good reason, so PR closed =]

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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