drupal_cron_cleanup uses the cron_semaphore variable which was removed in #805702: Cron should use locking framework .

Files: 
CommentFileSizeAuthor
#16 1775488-D7-drupal_cron_cleanup-16.patch526 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 39,891 pass(es).
[ View ]
#11 1775488-D7-drupal_cron_cleanup-11.patch1.16 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]
#8 1775488-drupal_cron_cleanup-8.patch1.08 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,278 pass(es).
[ View ]
#1 1775488.patch712 bytesamateescu
PASSED: [[SimpleTest]]: [MySQL] 41,102 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new712 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,102 pass(es).
[ View ]

Something like this, perhaps?

that simple and right i think, but if variable has been removed. will it make sense to remove it also in lock() ?

That's a different variable :)

Fix looks good, interesting how long it took to find this :)

I'm wondering if we can add test coverage for this. Maybe add a cron implementation that does exit() which we then call through a php request? (drupal_run_cron() obviously won't work). Then we can check that the lock is gone and we have a watchdog entry...

We only found this because #1175054: Add a storage (API) for persistent non-configuration state mentioned cron semaphore and when I tried to convert it I found it doesn't exist any more...

I'm not sure that the shutdown function is even necessary anymore.

  // Try to allocate enough time to run all the hook_cron implementations.
  drupal_set_time_limit(240);
  $return = FALSE;
  // Grab the defined cron queues.
  $queues = module_invoke_all('queue_info');
  drupal_alter('queue_info', $queues);
  // Try to acquire cron lock.
  if (!lock()->acquire('cron', 240.0)) {
    // Cron is still running normally.
    watchdog('cron', 'Attempting to re-run cron while it is already running.', array(), WATCHDOG_WARNING);
  }
  else {
    // Run cron...

Doesn't this mean that after 240 seconds we'll be able to reacquire the lock regardless? And wasn't the issue with the semaphore variable that if cron crashed then the semaphore variable had to be manually reset so that you could run cron again? The shutdown function was solving this and hence, is no longer needed since the lock will be released.

Status:Needs review» Needs work

Yes, lock does a releaseAll, so it's quite possible that we don't need this anymore, the only problem would be that we'd loose that watchdog message. But no 7.x site on this world has ever seen this watchdog message, so let's just remove the whole function.

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 49,278 pass(es).
[ View ]

I hope, understand the discussion proper, we are planning to remove cleanup as it is not necessary anymore(because of lock does a reaseAll). If it is true, here is the patch for this cleanup. Also can we rename the issue something like "Removing drupal_cron_cleanup"?

Status:Needs review» Reviewed & tested by the community

Yes, I think we can remove this. The only thing we lose is that watchdog message, which is misleading anyway, see #308808: Drupal_cron_cleanup paints the wrong picture.

Also marked a bunch of issues as duplicate:
- #302739: Watchdog interferes with cron semaphore deletion in drupal_cron_cleanup()
- #1777162: drupal_cron_cleanup() does nothing due to legacy code
- #942256: Remove drupal_cron_cleanup leftover since we moved to the lock api

Last chance for anyone to disagree :)

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]

Here is 7.x patch.

Status:Needs review» Reviewed & tested by the community

Backport looks good. As explain above, this never did anything in 7.x because there is no such variable anymore.

Status:Reviewed & tested by the community» Needs review

Ugh, that's unfortunate.

For Drupal 7, shouldn't this leave the function itself around though (and mark it as deprecated) rather than completely removing it?

Hm, good point, we could do that. Not sure if we should keep the function as is or replace the function body with just a comment.

Seems like it would be simpler to just keep the function body in place?

I mean, if someone out there is calling it directly (which is unlikely in the first place), it's possible they're doing something else non-standard such that it actually works - I doubt it, but you never know :)

StatusFileSize
new526 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,891 pass(es).
[ View ]

Updating patch to remove call to drupal_register_shutdown_function('drupal_cron_cleanup');

Can we also add a @deprecated to that function and say that it is not used and only kept in place for backwards compatibility?