Handle exceptions thrown in child processes forked by ProcessManager

[pawel-siejba]

Description (*)

The following bug can have fatal consequences if somebody employed any form of exception handling around indexers in their installation, as it could lead to very hard to discover bugs, that appear only on environments with several threads available, and MAGE_INDEXER_THREADS_COUNT env variable set.

As of now in Vanilla Magento 3 indexers(catalogsearch_fulltext, catalog_category_product, catalog_product_price) use the process manager for splitting index dimensions into separate processes.

If one of the subprocesses fails, it's not exited immediately, instead the exception continues through parent's execution flow

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Process Manager doesn't handle exceptions properly in forked processes #30622

Manual testing scenarios (*)

1. Run MAGE_INDEXER_THREADS_COUNT=4 php bin/magento indexer:reindex catalogsearch_fulltext
2. Kill Elasticsearch process to simulate an exception thrown or hard code an exception in the callables passed to ProcessManager

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @pawel-siejba. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[ihor-sviziev]

@magento run all tests

[pawel-siejba]

Can be easily back ported all the way to 2.3.0 Without any changes up to 2.3.2, 2.3.0 - 2.3.1 have some phpcs comments removed, but the class stays intact.

[ihor-sviziev]

@pawel-siejba
Your changes looks good. Let's wait for test results.
Will you be able to cover your change with any type of automated tests?

[pawel-siejba]

@magento run Static Tests

[pawel-siejba]

@ihor-sviziev I'll try to write some Unit Tests but I'm not sure if PHPUnit will handle that well as we will be essentially testing process forking.

[pawel-siejba]

@magento run all tests

[pawel-siejba]

@magento run Functional Tests CE, Functional Tests EE, Functional Tests B2B

[pawel-siejba]

@ihor-sviziev It doesn't look like writing Unit tests in this case will be feasible. Additional extensions installed may be required, on top of that it looks to be impossible to Unit test this implementation of Process manager in a way that would prove useful to this PR - without mocking/replacing the pcntl extension function calls. Moreover forking inside a Unit test doesn't seem like the best idea to me.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8375 has been created to process this Pull Request

[engcom-Delta]

@pawel-siejba with new changes we have same output as before but it is recorded into exception.log file. Could you clarify if this is expected behavior with your fix?
Manual testing scenario:

• Create additional website with products
  
• Run MAGE_INDEXER_THREADS_COUNT=4 php bin/magento indexer:reindex catalogsearch_fulltext
• Kill Elasticsearch process to simulate an exception thrown (e.g., kill pid command)

Output on 2.4-develop:

Output with fix in console:

Output with fix in log file:
exception.log

[pawel-siejba]

@engcom-Delta Yes, this is the expected behaviour. BTW don't know if you noticed but you unveiled another bug with the ProcessManager that I will create a separate issue for as it's not related to this change and also reproducible without this change. Basically if MAGE_INDEXER_THREADS_COUNT > Number of dimensions(in your case 3 stores) the main process always exits successfully.

#30964

[engcom-Delta]

@pawel-siejba thanks for clarification
@ihor-sviziev Could you approve to move PR into ready for testing column?

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8375 has been created to process this Pull Request

[engcom-Delta]

✔️ QA passed
Was checked case from Manual testing scenarios
Before:
❌ Errors are shown in the console

After:
✔️ Errors are recorded into log file

exception.log:

[2020-11-19 16:24:48] main.ERROR: Child process failed with message: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>405 Method Not Allowed</title>
</head><body>
<h1>Method Not Allowed</h1>
<p>The requested method PUT is not allowed for this URL.</p>
<hr>
<address>Apache/2.4.41 (Ubuntu) Server at localhost Port 80</address>
</body></html>
 {"exception":"[object] (Elasticsearch\\Common\\Exceptions\\BadRequest400Exception(code: 405): <!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">
<html><head>
<title>405 Method Not Allowed</title>
</head><body>
<h1>Method Not Allowed</h1>
<p>The requested method PUT is not allowed for this URL.</p>
<hr>
<address>Apache/2.4.41 (Ubuntu) Server at localhost Port 80</address>
</body></html>
 at /var/www/html/magento2dev/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:632)"} []
[2020-11-19 16:24:48] main.ERROR: Child process failed with message: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>405 Method Not Allowed</title>
</head><body>
<h1>Method Not Allowed</h1>
<p>The requested method PUT is not allowed for this URL.</p>
<hr>
<address>Apache/2.4.41 (Ubuntu) Server at localhost Port 80</address>
</body></html>
 {"exception":"[object] (Elasticsearch\\Common\\Exceptions\\BadRequest400Exception(code: 405): <!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">
<html><head>
<title>405 Method Not Allowed</title>
</head><body>
<h1>Method Not Allowed</h1>
<p>The requested method PUT is not allowed for this URL.</p>
<hr>
<address>Apache/2.4.41 (Ubuntu) Server at localhost Port 80</address>
</body></html>
 at /var/www/html/magento2dev/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:632)"} []

Note: Issue with the main process always exits successfully is reproducible on 2.4-develop without fix:
#30626 (comment)

[engcom-Delta]

Note: Automation tests are passed

[m2-assistant]

Hi @pawel-siejba, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.