Add drupal_cron_is_running()

Somehow related: #1217668: drupal_path_initialize() should not call drupal_get_normal_path() when run from cron.php.

Could be only cron_running, to avoid being confused with another hook...

Triggering test to make issue alive

I renamed the function to fit the pattern and re-rolled the patch

Would be nice to have some tests here to ensure cron runs code that has drupal_goto().

#5: 888998-drupal-cron-is-running.patch queued for re-testing.

Re-rolling

Some mechanism like this seems like a really good idea. I'm hesitant about adding it to the globals, though. Maybe someone smarter than me can weigh in on that.

Here's a couple of minor cleanups.

+++ b/includes/common.incundefined
@@ -5083,6 +5089,23 @@ function drupal_cron_run() {
+ * Check if cron hooks are being called and queues are being run.

Minor thing, but I'd change this to "Checks whether cron hooks are being called and queues are being run."

+++ b/includes/common.incundefined
@@ -5083,6 +5089,23 @@ function drupal_cron_run() {
+ *     - queues are being processed right now.

And let's capitalized "Queues" here, I think.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch, including fixing the two points I describe above.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Except there was a validation error and I failed to set the status or the tag. Oops.

Patch for #10 (sans any mod to the concept of adding it to the globals).

fixed spaces introduced in #12

Yes, I was thinking, and leaving the global also did not make sense if using a function, so I made it a static variable, not sure if that was the best way of implementing it.

Why not kill the function entirely and do:

variable_set('cron_running', TRUE);
// Run cron.
...
variable_det('cron_running');

Then instead of

if (drupal_cron_is_running()) {
 ...
}

people can do

if (variable_get('cron_running', FALSE)) {
  ...
}

??
I think that would be a little less overhead code-wise.

#15 seems like a better solution to me as well, especially since memory overhead is a concern on cron. I'll roll a patch and test with that method.

Mmm, I don't like the idea. This value is supposed to be used to determine if the CURRENT request is a cron run.

variable_set() will cause all parallel requests to believe they're being run on a cron.

Also, memory overhead is minimal (couple of lines of code and a static variable?) and O(1), so it's not that much of a concern.

#17: Oh, that is an excellent point. Doh. So #14 is the correct solution, then.

I'll rewrite my test for #14 instead.

Attached test-only patch tests whether drupal_cron_is_running() returns the expected value at various points. (Note: The patch includes drupal_cron_is_running() to avoid undefined function fatals, but does not use it during cron.) Should fail.

+++ b/core/modules/simpletest/tests/common_test.moduleundefined
@@ -251,12 +251,32 @@ function common_test_js_and_css_querystring() {
\ No newline at end of file

Whoops. I'll fix this in the combined patch. This won't affect the demonstration that the test fails, though. :P

+++ b/core/includes/common.inc
@@ -5207,6 +5213,30 @@ function drupal_cron_run() {
+ * @param $value - If not null, set the boolean value of the variable

Just noticed this is not conforming to doc standards... Should be a newline sentence with a period.

23 days to next Drupal core point release.

#22 Did you look at the most recent patch? I already changed that.

Still waiting on testbot here before I upload the combined.

'Bout time. Fails on the correct assertion. Now, the combined patch. This is the one to review.

No, sorry. Test failed fine, we're almost done here. =)

Ops, cross-post...

+++ b/core/includes/common.incundefined
@@ -5207,6 +5213,32 @@ function drupal_cron_run() {
+ *   - Queues are being processed right now.

Oh, I just realized that I didn't add test coverage for this case.

Oh, wait. Actually, I think that this functionality hasn't been added yet. I think we'd need to add setting and unsetting the flag around the bit below? Can anyone weigh in on whether this is correct (and something we want to do) before I reroll anything again?

  foreach ($queues as $queue_name => $info) {
    $function = $info['worker callback'];
    $end = time() + (isset($info['time']) ? $info['time'] : 15);
    $queue = DrupalQueue::get($queue_name);
    while (time() < $end && ($item = $queue->claimItem())) {
      $function($item->data);
      $queue->deleteItem($item);
    }
  }

Do you mean setting the same flag when processing any queue?

#29: Hmm sorry, I don't understand the question. Maybe too early in the morning. :)

This is what I mean. Change the snippet in #28 to:

  // Flag that queued cron items are currently being run.
  drupal_cron_is_running(TRUE);
  foreach ($queues as $queue_name => $info) {
    $function = $info['worker callback'];
    $end = time() + (isset($info['time']) ? $info['time'] : 15);
    $queue = DrupalQueue::get($queue_name);
    while (time() < $end && ($item = $queue->claimItem())) {
      $function($item->data);
      $queue->deleteItem($item);
    }
  }
  // Flag that queued cron items are no longer being run.
  drupal_cron_is_running(FALSE);

The comments for drupal_cron_is_running() appear to claim that we're doing this, but we're not yet.

Yes, that's it, I guess this makes it complete.

Alright, will try to add that and come up with some test coverage for it.

So, three tasks:

1. Reroll the patch to apply against current 8.x. Note that files have moved around; common_test.module is now inside core/modules/system/tests and CronRunTestCase will have been renamed and moved for PSR-0; look in core/modules/system/lib/Drupal/system/tests//
2. Update the patch with the snippet in #30.
3. Add additional test coverage for when the flag should be TRUE.

I've re-rolled the patch and added drupal_cron_is_running() around the queue code (tasks 1 and 2 from #33).

Task "3. Add additional test coverage for when the flag should be TRUE." from #33 still needs to be addressed.

Whoops - patch fixed now, plus an interdiff.

Here's a few more details about my patch. It patch contains two things: (1) the reroll to reflect the changed file structure and line numbers of the PSR-0 testing scheme, and (2) the added code to use the new drupal_cron_is_running() function around the processing of queued cron tasks.

The interdiff shows the changes between the reroll and the added code (no interdiff for the reroll).

reroll!

#38: drupal-cron_is_running-888998-38.patch queued for re-testing.

received these same exact failures in 1209674. Appears to be a testbot fluke. Resending.

Some minor improvement suggestions, but all in all I think this warrants a "needs work".

+++ b/core/includes/common.inc
@@ -5068,6 +5081,32 @@ function drupal_cron_run() {
+ * @param $value

I couldn't really come up with a better suggestion, but $value seems random.

+++ b/core/includes/common.inc
@@ -5068,6 +5081,32 @@ function drupal_cron_run() {
+ *   (optional) A boolean flag to indicate whether cron is running, or NULL to
+ *   return the saved value.  Defaults to NULL.

This makes it sound like you should explicitly pass NULL instead of just omitting the variable alltogether. I think we could just omit the second clause in the first sentence.

+++ b/core/includes/common.inc
@@ -5068,6 +5081,32 @@ function drupal_cron_run() {
+ *   - Queues are being processed right now.

I think this should mention that this only applies to cron queues, i.e. queues that have set the cron key.

+++ b/core/includes/common.inc
@@ -5068,6 +5081,32 @@ function drupal_cron_run() {
+  static $drupal_cron_is_running;
+
+  if (!isset($drupal_cron_is_running)) {
+    $drupal_cron_is_running = FALSE;
+  }

This should be simply $drupal_cron_is_running = FALSE;

That said, the variable is too verbose. The drupal part is superfluous at the very least.

Addressed #42

+++ b/core/includes/common.inc
@@ -5068,6 +5081,28 @@ function drupal_cron_run() {
+    $cron_is_running = $value;

I was gonna mark this RTBC, but the bot is smarter than me...
This should be $running as well.

Same here. I think we already have tests.

+++ b/core/modules/system/tests/modules/common_test/common_test.moduleundefined
@@ -340,3 +350,14 @@ function common_test_l_active_class() {
+  variable_set($var, drupal_cron_is_running());

Any reason why we introduce a variable here? it should be config

The reason is that the patch is older than those changes.

I'm still confused by the new API. Should this be converted to a state or a config?

ah..yes you are right, it should be state

Let's see now.

Forgot the variable_get(). Done right this time, hopefully.

Looks great!

I'm not sure about this. Most of the examples involve doing things like drupal_goto(), which is almost always a bad idea (since it completely halts script execution). This could go wrong for anything invoked by drush, not necessarily just cron.

Also I'd expect this to tell me whether cron is running on the site at all with the current name - drush ev "print drupal_cron_is_running()" for example.

re #57... it is meant exactly for that, if it doesn't work, needs to be fixed.

I see what you mean, I forgot the original idea about it. We need to set a state there.

After reviewing the discussion, I read myself saying that no, the purpose is exactly to tell whether cron is running on current call, so that stuff running inside cron jobs will know it and do things differently.

Novice issue cleanup.

54: drupal-cron_is_running-888998_0.patch queued for re-testing.

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Will give this one more bump, if no interest will close in 3 months, thanks all!

This had a lot of comments so giving 1 final bump.

With

    if (!$this->lock->acquire('cron', 900.0)) {
      // Cron is still running normally.
      $this->logger->warning('Attempting to re-run cron while it is already running.');
    }

Is this still needed?

I'm not sure it's needed as mentioned in previous comments some of the scenarios aren't advised anyway.

If its something seen as a good thing to have, I'd favour adding a method to the cron service rather than checking the lock. Another module could change the cron service and use a different way of setting that cron is running.