- 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
andset_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'sset_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 …
set_time_limit()
may be disabled by the site administrator — see the docblock on\Drupal\Component\Utility\Environment::setTimeLimit()
- 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:
- a new validator that verifies that
set_time_limit()
is available - 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 callslocale_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:
- 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 .
- Get a mechanism in place to ensure a
- 🇮🇳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