DI compilation crashes when composer classmap includes generated code

scottsb commented 5 years ago

Preconditions (*)

Have the following in your project composer.json as recommended since #16435 was merged:

    "psr-0": {
        "": [
            "app/code/",
            "generated/code/"
        ]
    },

Reproduced on Magento 2.3.1, but if those lines are in your composer.json file the issue will apply to many older versions as well since the relevant code hasn't changed for a long time.

Steps to reproduce (*)

Run the following commands in sequence:

bin/magento setup:di:compile
composer dump-autoload -o
bin/magento setup:di:compile

Note that this is a minimal sequence to reproduce the issue. Normally this would be seen with steps #1-2 run during one deploy and then step #3 run the next deploy.

Expected result (*)

Compilation succeeds both times.

Actual result (*)

Second compilation command fails with an error like this:

Warning: include(/var/www/html/vendor/composer/../../generated/code/Magento/Framework/App/ResourceConnection/Proxy.php):
failed to open stream: No such file or directory in /var/www/html/vendor/composer/ClassLoader.php on line 444

Analysis

The issue is that the core Cli module includes magic early behavior to clear out the environment when a compilation command is being run, including deleting the generated code directories (see method Cli::assertCompilerPreparation() and note it's called before Cli::initObjectManager()). However, when the object manager is then initialized it tries to load classes from the Composer classmap that no longer exist, since they were in the deleted generated directories.

The method assertCompilerPreparation() has the following comment:

    /**
     * Temporary workaround until the compiler is able to clear the generation directory
     * @todo remove after MAGETWO-44493 resolved
     */

I don't know what MAGETWO-44493 is, but it appears that the DiCompileCommand class does indeed already clear the generation directory itself (futilely since it's already cleared by that point), so the comment is at a minimum misleading.

Possible solutions:

Refactor the magic deletions out of the generic Cli class and into the relevant command classes after the object manager is initialized. This would be ideal, but I assume part of the goal is to prevent the object manager from loading old generated code, so I'm not sure how feasible this is.
Leave the existing magic as it is and add additional magic to remove or replace the Composer autoloader when compiling to avoid trying to access deleted generated code, thereby skipping straight to dynamic generation.

Workaround

Until this is fixed the error can be avoided by manually deleting the generated directory contents and then triggering a temporary classmap that excludes generated (either by running composer install or a simple composer dump-autoload) before running DI compilation. (Credit for the workaround and helping along my investigation to jalogut/magento2-deployer-plus#29, which I came across when Googling this problem.)

m2-assistant[bot] commented 5 years ago

Hi @scottsb. Thank you for your report. To help us process this issue please make sure that you provided the following information:

[x] Summary of the issue
[x] Information on your environment
[x] Steps to reproduce
[x] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

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

@scottsb do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

[x] yes
[ ] no

Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] commented 5 years ago

Hi @engcom-qa-Charlie. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
Details
If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.
[ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 6. Add label Issue: Confirmed once verification is complete.
[ ] 7. Make sure that automatic system confirms that report has been added to the backlog.

magento-engcom-team commented 5 years ago

:white_check_mark: Confirmed by @engcom-qa-Charlie Thank you for verifying the issue. Based on the provided information internal tickets MC-17500, MC-17501 were created

Issue Available: @engcom-qa-Charlie, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

fascinosum commented 4 years ago

Hi @scottsb, we recommend the following deployment flow:

composer install
bin/magento setup:di:compile
composer dump-autoload -o

We are not going to add additional magic to handle the case without composer install

scottsb commented 4 years ago

@fascinosum The presence or absence of composer install isn't really related to this bug. In actual fact, we do run composer install before bin/magento setup:di:compile and have done so all along, including when we first discovered this bug. It wasn't mentioned in the issue description because I was providing only the minimal necessary steps to trigger the bug.

fascinosum commented 4 years ago

HI @scottsb, we are clearly understand what this issue is about. You are absolutely right, if we use the commands from the description, we will encounter the error. Please use the following order of commands for each you deployment

composer install
bin/magento setup:di:compile
composer dump-autoload -o

As you know, composer dump-autoload -o is not a part of any Magento CLI command. If you use this instruction, you should clear composer cache using composer install before the bin/magento setup:di:compile command. Please let us know if the issue is still reproducible with the recommended flow

stkrelax commented 4 years ago

We have a similar issue but with module:disable

same problem as @scottsb said. Thanks for the Input btw...

We found the one resposible for this. https://github.com/magento/magento2/blob/f3a160c2dc7a883dffdfbb5f268fa87087b639a5/setup/src/Magento/Setup/Console/CompilerPreparation.php#L91

PascalBrouwers commented 4 years ago

As you know, composer dump-autoload -o is not a part of any Magento CLI command.

What's the point in selling cars if you've never driven one? Yes, people use "composer dump-autoload -o" when they deploy to a server.

Bug is easily reproducible

bin/magento c:f
composer install --optimize-autoloader
bin/magento -> here I get the error
composer install --optimize-autoloader
bin/magento -> everything fine again

bin/magento c:f
composer install --optimize-autoloader
bin/magento -> there is that error again

hostep commented 4 years ago

@PascalBrouwers and @scottsb: those aren't realistic scenario's for deployments I think.

Can you list more detailed steps for how to reproduce this in a realistic deployment?

We are doing the following with each deploy, every time starting from an empty directory, we had no issues so far with a broken autoloader:

composer install --optimize-autoloader --prefer-dist --no-dev
bin/magento deploy:mode:set --skip-compilation production
bin/magento setup:di:compile
composer dump-autoload --optimize --no-dev
bin/magento setup:static-content:deploy  .....
bin/magento maintenance:enable
bin/magento setup:upgrade --keep-generated
bin/magento maintenance:disable

On a local installation you can indeed very quickly trigger the bug, but you shouldn't use an optimised autoloader on a local development installation because the generated code can change so often. It's not recommended to use an optimised autoloader in such cases.

PascalBrouwers commented 4 years ago

Yes, I am talking about a deployment starting from an empty directory.

I'm just describing how easy it is to reproduce this issue. I am not going to list the whole set of commands used to deploy. Just use capistrano, deployer, etc.

But as you've mentioned: "On a local installation you can indeed very quickly trigger the bug". Also, @scottsb provided a very in-depth analysis of where this bug is coming from.

Zyles commented 4 years ago

What is the solution? I would like to deploy our Magento 2 installation.

Now we are just sitting here wasting time like always with Magento.

hostep commented 4 years ago

@Zyles: you need to be more specific: what steps do you take and when does it fail? Also: have you read the recommendations mentioned above in various comments?

Zyles commented 4 years ago

Ya I spend 2 hours trying to get compilation working.

$ composer update
$ rm -rf generated/*
$ php bin/magento setup:di:compile

Compilation was started.
Interceptors generation... 4/7 [================>-----------]  57% 15 secs 259.0 MiB

Also tried deleting the vendor dir and doing composer install with same problem.

Never had issues before, but as always random errors with Magento 2 is common and things breaking for no reason.

Apparently it is impossible to get any verbose feedback from setup:di:compile so who knows what is happening?

hostep commented 4 years ago

I'm not seeing any error in your output. Is there a specific error? Are you sure you are running into the issue this ticket is about?

You should get an error like this Class Magento\Framework\App\ResourceConnection\Proxy does not exist (Magento 2.3.6) or Warning: include(vendor/composer/../../generated/code/Magento/Framework/App/ResourceConnection/Proxy.php): failed to open stream: No such file or directory in vendor/composer/ClassLoader.php on line 444 (Magento 2.4.1)

You can always fix this issue by just running composer install (so without an optimised autoloader)

If this isn't the issue you are running against, I suggest you search help elsewhere 🙂

scottsb commented 4 years ago

@hostep When I first ran into this issue, it was not on a local environment: it was via our house-built deploy scripts that we use for staging and production deploys. As referenced in the issue, it also came up for M2 Deployer Plus, a reasonably popular open source set of deploy scripts. So while it's true that you shouldn't use an optimized autoloader locally, that's also somewhat non sequitur.

As far as providing a "realistic" set of deploy commands, that seems to be to be rather pointless, as it all it would do is add noise to the actual relevant steps. I mentioned in the issue description that it is the minimal reproducible example. I will point out this from the description in case you had skimmed past it originally, as it is key to seeing how this could be a "realistic" scenario:

Normally this would be seen with steps #1-2 run during one deploy and then step #3 run the next deploy.

Note that unlike @PascalBrouwers we were not deploying on an empty directory. But unless it becomes an officially documented requirement to deploy only on an empty directory, that's not sufficient reason to disregard this issue.

lbajsarowicz commented 3 years ago

I was able to reproduce the issue in Magento 2.4.2. Build process fails on seutp:di:compile

composer install --prefer-dist --no-interaction --no-dev --optimize-autoloader \
    && find $RELEASE_PATH -type d ! -perm 2770 -exec chmod 2770 {} + \
    && find $RELEASE_PATH -type f ! -perm 0660 -exec chmod 0660 {} + \
    && chmod +x $RELEASE_PATH/bin/magento \
    && bin/magento setup:di:compile \
    && bin/magento setup:static-content:deploy \
    && composer dump-autoload --no-dev --optimize

hostep commented 3 years ago

@lbajsarowicz: you can just remove --optimize-autoloader from your composer install command. The autoloader will get optimised by your dump-autoload command a bit later.

lbajsarowicz commented 3 years ago

@hostep Looks like it works (however, I tried that before and for some reason, it didn't work). Thank you.

lbajsarowicz commented 3 years ago

I always wondered if anyone is doing Code Review of Adobe Core developer changes.

Let's look at the "fixes" introduced in Magento 2.4.2: https://github.com/magento/magento2/commit/23c61bd49b5323d08f2cd4592ee451ca03e4b9e1

This is strictly related to the issue we are encountering (Look at the references to non-existing classes like \Magento\Framework\App\ResourceConnectionConnection)

PivitParkour94 commented 3 years ago

So it seems like using the built in optimiser for composer is unsupported for M2.4.2 at least? Yet this isn't a confirmed bug? Just wondering what further information is needed, other than what @lbajsarowicz has referenced in the commit?

The fact that the response to using a command outside of the standard M2 process to setup a store is shut down as "As you know, composer dump-autoload -o is not a part of any Magento CLI command" is unfortunate, I thought it would be in Magento's best interest to try and accommodate as many different ways to get their code working as possible, in order for more people to have success and a good experience when using Magento 2.

Thanks for the useful information regardless. Hopefully things can improve and we can find a good deployment solution for local dev, staging and production one day :)

ioweb-gr commented 3 years ago

I'm just writing here my experience regarding this.

In our websites without running

composer dump-autoload -o --apcu

we're getting 1.5 seconds slower execution speeds due to file loading by composer.

By running just

composer dump-autoload -o

we're getting errors like above.

It seems in devdocs https://devdocs.magento.com/guides/v2.4/performance-best-practices/deployment-flow.html there's some documentation on how to deploy composer optimized via using apcu cache, but it's not really well visible and it doesn't explain why it works only with apcu cache enabled.

I would expect not having to use apcu would result the same as using apcu

Now the only working order for us is after installing dependencies we execute deployment commands in this order

bin/magento s:d:c
composer dump-autoload -o --apcu
bin/magento s:s:d

Any other attempt with optimized autoloader breaks everything for us.

kandy commented 3 years ago

I don't think that "Expected result" is proper. Based on composer documentation

Note: You should not enable any of these optimizations in development as they all will cause various problems when adding/removing classes. The performance gains are not worth the trouble in a development setting.

and "setup:di:compile" command certainly falls under this definition

So, maybe best way to document this behavior on devdocs (https://devdocs.magento.com/guides/v2.4/performance-best-practices/deployment-flow.htm we recommend to run composer dump-autoload -o after ' s:d:c' for example)

Can you share your opinion on it?

ioweb-gr commented 3 years ago

So, maybe best way to document this behavior on devdocs (https://devdocs.magento.com/guides/v2.4/performance-best-practices/deployment-flow.htm we recommend to run composer dump-autoload -o after ' s:d:c' for example)

Regarding this if I don't add --apcu I still get errors during execution. It only seems to work with apcu enabled.

scottsb commented 3 years ago

@kandy I don't follow this:

Note: You should not enable any of these optimizations in development as they all will cause various problems when adding/removing classes. The performance gains are not worth the trouble in a development setting.

and "setup:di:compile" command certainly falls under this definition

The only time you should be running setup:di:compile is when building for production (as was my case in the original report scenario). You should not normally run that in development (when Magento should normally be in developer mode). Therefore, the note you quote from the Composer docs seems non sequitur.

Can you please clarify?

kandy commented 3 years ago

@scottsb the important part is "various problems when adding/removing classes." and setup:di:compile add Factory, Proxy, etc classes

scottsb commented 3 years ago

@kandy Ah, I understand what you were saying now. However, note this from my original report:

This is a minimal sequence to reproduce the issue. Normally this would be seen with steps #1-2 run during one deploy and then step #3 run the next deploy.

In other words, I was already "running composer dump-autoload -o after s:d:c" as you suggest documenting (that's steps 1-2 in my minimal case). The issue would come up on the next deploy, when you try to run DI compilation (step 3 in my minimal case). Now it fails, though, because the class map is still dumped from the last deploy.

Would you be able to share what MAGETWO-44493 is? That would help shed light on the root of the issue here, since per the code comment that was supposed to allow the problematic code to be deleted.

kandy commented 3 years ago

@scottsb I recommend to de-optimize autoloader by running composer dump-autoload on the first step of your build or even better to run build from scratch every time

MAGETWO-44493 is about making compilers generate code without relying on autoloading, and it's in an Obsolete state now

colin-redbox commented 2 years ago

Further to the discussion taking place here, I can confirm that the following works in 2.4.3 and makes a significant performance improvement. The autoloader is added to OpCache following this command, I haven't tested it in earlier versions.

composer dump-autoload && bin/magento s:di:c && composer dump-autoload --optimize --no-dev --classmap-authoritative

magento / magento2