[META] Update doc comment on BatchProcessor to specify why use the batch system

Created on 2 May 2023, over 1 year ago
Updated 3 May 2023, over 1 year ago

Problem/Motivation

#3354701-12: Ensure exceptions thrown by event subscribers are logged follow-up to where @wim leers found it confusing why we don't just do the UI updates in 1 request like cron updates do.

Proposed resolution

Update the doc comment in \Drupal\automatic_updates\BatchProcessor to specify why we use the batch system

  1. Cron sets a long time limit to be able to do more operations. We could do this in the UI but then user has no feedback for the entire cycle.
  2. The batch system gives the user feedback that something is happening so they don't have to wait and see nothing for whole cycle.
  3. After the update is staged and before it is applied in UI we are able to show the user the form with the "Continue". Here we can show them more relevant information about what was staged. Currently I think all we are showing them is the message about possible database updates from StagedDBUpdateValidator but this is very important information and the user may decide not to run an update that has staged database updates(to run it at another time). Other contrib or custom code also follow this same pattern of listening to StatusCheckEvent and if there is staged update show some warning if it is not cron and an error if it is cron.

    In cron updates we don't allow updates to run if there are staged database updates. There wouldn't be anybody to show this information to anyways.

Remaining tasks

📌 Task
Status

Active

Version

3.0

Component

Code

Created by

🇺🇸United States tedbow Ithaca, NY, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @tedbow
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    High-level observation

    🤔 Is it really safe to do the whole stage life cycle in a single request during cron updates, but using multiple requests (because batch) while using the UI?

    Crash in real world

    Thanks for creating this issue, @tedbow. Earlier today, while pairing with @Omkar.Podey, I actually SAW IT CRASH MID-REQUEST, and then it was impossible to delete the stage using the UI, because it was presumed to still be applying! (see \Drupal\package_manager\StageBase::isApplying()) 😨

    (To be fair, XDebug was on, but even without XDebug, we have to assume at least the network could be slow. Fetching a Drupal core update is multiple megabytes.)

    PHP's max_execution_time and set_time_limit()

    PHP's max_execution_time defaults to 30s, which is really risky/close. 😱 Even for batch processing this is risky; for this very reason e.g. the migration system has special measures in place: \Drupal\migrate_drupal_ui\Batch\MigrateUpgradeImportBatch::run() — introduced by #2281691: User interface for migration-based upgrades .

    The issue summary says:

    Cron sets a long time limit to be able to do more operations. We could do this in the UI but then user has no feedback for the entire cycle.

    But I was unable to find it doing anything relating to max_execution_time. I dug deeper. Apparently \Drupal\Core\Cron::run calls \Drupal\Component\Utility\Environment::setTimeLimit() with $time_limit = 240, which in turn calls PHP's set_time_limit() (so: 4 minutes instead of the default of 0.5).

    TIL!

    php-tuf/composer-stager

    And not only Drupal core's Cron class uses it, so do \PhpTuf\ComposerStager\Infrastructure\Service\FileSyncer\PhpFileSyncer::sync() and \PhpTuf\ComposerStager\Infrastructure\Service\FileSyncer\RsyncFileSyncer::sync().

    Limitations of set_time_limit()

    But …

    1. set_time_limit() may be disabled by the site administrator — see the docblock on \Drupal\Component\Utility\Environment::setTimeLimit()
    2. even if it is available, it can only be used to extend the time limit so much: mod_fastcgi, mod_fcgid, php-fpm etc. all have their own time limits too!

    Conclusion

    So I think we need:

    1. a new validator that verifies that set_time_limit() is available
    2. a mechanism that ensures that the stage life cycle is executed across multiple cron requests — we should probably look at examples in core like locale_cron() which only calls locale_cron_fill_queue() and then has a corrresponding @QueueWorker plugin: \Drupal\locale\Plugin\QueueWorker\LocaleTranslation

    More investigation needed. But given how this will very likely result in problems on real-world test sites, bumping to .

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    In addition to the 2 issues above:

    1. Get a mechanism in place to ensure a FailureMarker gets created if the PHP execution time is exceeded — without that, there's just a silent failure which is nearly impossible to debug. That's what @Omkar.Podey ran into over at 📌 [PP-1] Implement experimental cron updates for contrib in automatic_updates_extensions Postponed .
  • 🇮🇳India omkar.podey

    for reference to the

    impossible to delete stage

    in #2 📌 [META] Update doc comment on BatchProcessor to specify why use the batch system Active

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Issue for the third follow-up: 📌 Write helpful message to FailureMarker if script execution time was exceeded during stage life cycle Postponed: needs info .

    I think we should convert this to a meta, since it's not clear how to document all this until the follow-ups are done? 🤔 Tentatively retitling to and recategorizing as .

  • 🇺🇸United States tedbow Ithaca, NY, USA

    created 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed

    See the issue for detail but I am not positive we should be using cron at all

Production build 0.71.5 2024