Problem/Motivation

Per #3357632-3: [META] Update doc comment on BatchProcessor to specify why use the batch system.

Steps to reproduce

Install an update and make it take a very long time, e.g. by having a LOT of files in place, or by reducing the max_execution_time.

Proposed resolution

Detect max execution time being exceeded and write a helpful message to the failure marker.

Remaining tasks

  1. Experiment to find an approach that works — see #3 and , or via debugger:
  2. Write code
  3. Manual test
  4. Maybe an automated test?

User interface changes

A helpful message will now appear if either there's too much I/O (lots of updates) or too slow I/O (slow network or filesystem)

API changes

None.

Data model changes

None.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Assigned: Unassigned » wim leers
Issue summary: View changes
Status: Active » Needs work
StatusFileSize
new1.1 KB

Tricky: sleep() does not count against execution time! 🤯

wim leers’s picture

Issue summary: View changes
StatusFileSize
new5.94 KB
wim leers’s picture

Issue summary: View changes
StatusFileSize
new368.26 KB
wim leers’s picture

Issue summary: View changes

The commit I just pushed works in manual testing, but the automated test is very difficult to write. Will probably only finish that tomorrow.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs work » Needs review

Pushed what I got so far that AFAICT proves writing a Kernel test for this is impossible.

Maybe I can write a functional test for this, but I've not been very successful so far 🫠

Marking Needs review for the first commit.

tedbow’s picture

Assigned: Unassigned » tedbow

I need to review

tedbow’s picture

I don't think we want to use Failure marker for this. The class doc states

* The failure marker is a file placed in the active directory while staged
 * code is copied back into it, and then removed afterwards. This allows us to
 * know if a commit operation failed midway through, which could leave the site
 * code base in an indeterminate state -- which, in the worst case scenario,
 * might render Drupal being unable to boot.

So this is for a very particular purpose, "commit operation failed midway through, which could leave the site
* code base in an indeterminate state". I created #3360548: Document what the existence of the PACKAGE_MANAGER_FAILURE.yml means to make this clearer

I don't we should reuse it for this. We stop all operations if this file exists. The purpose of this file is that you can't trust your site's code at that point.

Of the 5 places we use this only during Apply would it leave your site's code base in an indeterminate state. But since it added to the first line of \Drupal\package_manager\StageBase::apply for all we know the time out is from very badly written PreApplyEvent subscriber. In that case we should not write the marker file. In StageBase::apply if call $this->committer->commit($stage_dir, $active_dir, $paths_to_exclude, NULL, $timeout); and we never return we will have already written the marker file and it will not be delete if the execution does not return to StageBase::apply. In that case still think we want to totally replace the file though in logExecutionTimeFailure as this MR does because it currently doesn't mention anything about restoring your site from a backup like \Drupal\package_manager\StageBase::getFailureMarkerMessage().

I will make some more specific suggestions in the MR

tedbow’s picture

Status: Needs review » Needs work

@Wim Leers can you update the summary to state concisely what the problem is we are trying to address. The Problem/Motivation section currently just links to another issue comment and that issue comment then mentions the 1 above it and then links to yet issue

tedbow’s picture

Status: Needs work » Postponed (maintainer needs more info)

Postponing this because re #10 I don't have the whole context of the problem space we are trying to solve

tedbow’s picture

Assigned: tedbow » wim leers