So, when deleting eck entities we have this drupal_cron_run() call in eck__entity__delete_form_submit.

  • This is emptying the whole drupal cache (which could be a problem, but if it us required there ure plenty of other way to delete the cache)
  • This is launching tasks in the cron that may be written for very long tasks and heavy memory usage. So chances are the form submit never ends (for the final user)

A lot of websites are deactivating the poor man's cron default behavior of drupal (running cron inside regular queries) and launch the cron via drush because of that.

No module should ever run drupal_cron_run()

CommentFileSizeAuthor
#10 eck-fix_drupal_cron_run_delete_form-1968734-10.patch334 bytesheddn
#8 1968734-8-remove_cron_run_on_delete.patch268 bytespounard
#1 eck-drupal_cron_run_on_delete_form_performance_problem-1968734-1.patch361 bytesregilero

Comments

regilero’s picture

regilero commented
Status: Active » Needs review
StatusFileSize
new eck-drupal_cron_run_on_delete_form_performance_problem-1968734-1.patch361 bytes

So here is a very simple patch to replace drupal_cron_run() by a drupal_flush_all_caches();

But do we need this cache flush? do we need some special cron to run? It this related to #1513952?

fmizzell’s picture

fmizzell commented

I can not think of any good reason of why that cron run should be there. I can imagine ECK calling cron when a bundle or entity type is deleted, since fields are softly deleted, and this can cause weirdness, but I can not think of any reason why cron should be run after deleting an entity. Let's get some reviews, but this should be an easy commit.

hughworm’s picture

hughworm commented

drupal_flush_all_caches() is still very heavy handed.
cache_clear_all() seems better.

tahiticlic’s picture

tahiticlic commented

Can someone explain why it is here ?

It's indeed a huge performance hole...

pounard’s picture

pounard
Primary language French
commented

No cache should be dropped on a single entity delete, except entitycache module cache if enabled and supported, and potentially field cache for the given entity, but that should be done implicitely if fields are handled well.

Sorry I was wrong. No cache delete at all is needed there, ECK module relies on Entity API module which will already do whatever cache needs to be dropped by itself. This line must just be removed.

fmizzell’s picture

fmizzell commented

When I first started working with ECK, I found some weird behavior coming from Field API, due to lingering data. If I remember correctly a cron run solved the problem, but that is an awful hack. It's been a couple of year since the initial ECK code was written, so I would say, let's just clean up stuff that doesn't seem to make sense and we will see is we get bug reports later.

pounard’s picture

pounard
Primary language French
commented

Agreed, this will resolve an unstainable performance problem. Entity API evolved a lot since and I trust it to be more stable now.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new 1968734-8-remove_cron_run_on_delete.patch268 bytes
pounard’s picture

pounard
Primary language French
commented

Did the patch using the 'diff' command, somehow I fear that the git branch may have diverged a lot from current stable release and needed to be able to patch our current running site.

Use patch -p0 < FILE to apply.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new eck-fix_drupal_cron_run_delete_form-1968734-10.patch334 bytes

Marking as RTBC. The re-roll is only trivial to apply cleanly against HEAD.

fmizzell’s picture

fmizzell commented
Status: Reviewed & tested by the community » Closed (fixed)

  • Commit 8e36728 on 7.x-2.x, 7.x-2.0.x, 7.x-2.1.x authored by heddn, committed by fmizzell: 
    issue #1968734 drupal_cron_run in eck__entity__delete_form_submit is a...

  • Commit 8e36728 on 7.x-2.x, 7.x-2.0.x, 7.x-2.1.x authored by heddn, committed by fmizzell: 
    issue #1968734 drupal_cron_run in eck__entity__delete_form_submit is a...

  • Commit 8e36728 on 7.x-2.x, 7.x-3.x authored by heddn, committed by fmizzell: 
    issue #1968734 drupal_cron_run in eck__entity__delete_form_submit is a...