New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console][DI] Fail gracefully #25255

Merged
merged 1 commit into from Dec 2, 2017

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Dec 1, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets symfony/flex#212, #25280
License MIT
Doc PR -

I already experienced this issue a few times without spending time digging it:
sometimes, you call cache:clear, and the command quits without any output, and with 255 status code.

The reason is the @include in Kernel, which makes everything silent, especially fatal errors (thanks PHP...)
So if the to-be-removed container is broken for some fatal reason, the failure is really bad.

To fix that, here are two measures:

  • use include_once instead of require_once in the dumped container: that's OK there to actually not immediately load the file, any hard failure will happen later anyway, and any soft failure will allow the cache:clear command to complete (like when you remove a package)
  • register Application::renderException() as the main PHP exception handler, via Debug::ErrorHandler when it's available

End result when it fails:
image

instead of a blank output.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Dec 1, 2017

Contributor

@nicolas-grekas so you would still get a silent hard failure if you do c:c --env prod?

Contributor

theofidry commented Dec 1, 2017

@nicolas-grekas so you would still get a silent hard failure if you do c:c --env prod?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

Not since 10 minutes. When did you read the code ? :)

Member

nicolas-grekas commented Dec 1, 2017

Not since 10 minutes. When did you read the code ? :)

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Dec 1, 2017

Contributor

I didn't, but I have trouble following :P

Contributor

theofidry commented Dec 1, 2017

I didn't, but I have trouble following :P

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 1, 2017

Member

Fwiw, I believe a user on Twitter hit the same issue - when removing Twig https://twitter.com/neelion333/status/936623941937795072 - looks very easy to reproduce

Member

weaverryan commented Dec 1, 2017

Fwiw, I believe a user on Twitter hit the same issue - when removing Twig https://twitter.com/neelion333/status/936623941937795072 - looks very easy to reproduce

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 2, 2017

Member

I think this bug might be quite critical. I think I'm even able to hit it through the web interface. While upgrading a project from 3.3 to 3.4, I did the following:

  1. Added the container.autowiring.strict_mode: true parameter.
  2. Refreshed to rebuild the container
  3. Removed this parameter
  4. Refreshed to this error:

screen shot 2017-12-01 at 10 40 10 pm

@nicolas-grekas Do you think it's related? Do you need me to try to create a repeater (the above was done on our big KnpU Symfony 3 tutorial project).

Member

weaverryan commented Dec 2, 2017

I think this bug might be quite critical. I think I'm even able to hit it through the web interface. While upgrading a project from 3.3 to 3.4, I did the following:

  1. Added the container.autowiring.strict_mode: true parameter.
  2. Refreshed to rebuild the container
  3. Removed this parameter
  4. Refreshed to this error:

screen shot 2017-12-01 at 10 40 10 pm

@nicolas-grekas Do you think it's related? Do you need me to try to create a repeater (the above was done on our big KnpU Symfony 3 tutorial project).

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 2, 2017

Member

@weaverryan looks like the same yes.

I improved the behavior by replacing the nasty @-silencing by a specific E_WARNING silencing. The container-loading file is also now resilient to a broken container folder, by using include+return in case of failure. Also handled by Kernel.

PR is ready. Dunno how to test it as the behavior is triggered by edge case fatal errors...

Member

nicolas-grekas commented Dec 2, 2017

@weaverryan looks like the same yes.

I improved the behavior by replacing the nasty @-silencing by a specific E_WARNING silencing. The container-loading file is also now resilient to a broken container folder, by using include+return in case of failure. Also handled by Kernel.

PR is ready. Dunno how to test it as the behavior is triggered by edge case fatal errors...

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 2, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Dec 2, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 4a5a3f5 into symfony:3.4 Dec 2, 2017

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Dec 2, 2017

bug #25255 [Console][DI] Fail gracefully (nicolas-grekas)
This PR was merged into the 3.4 branch.

Discussion
----------

[Console][DI] Fail gracefully

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/flex#212, #25280
| License       | MIT
| Doc PR        | -

I already experienced this issue a few times without spending time digging it:
sometimes, you call `cache:clear`, and the command quits without any output, and with 255 status code.

The reason is the `@include` in `Kernel`, which makes everything silent, especially fatal errors (thanks PHP...)
So if the to-be-removed container is broken for some fatal reason, the failure is really bad.

To fix that, here are two measures:
- use `include_once` instead of `require_once` in the dumped container: that's OK there to actually not immediately load the file, any hard failure will happen later anyway, and any soft failure will allow the `cache:clear` command to complete (like when you remove a package)
- register `Application::renderException()` as the main PHP exception handler, via `Debug::ErrorHandler` when it's available

End result when it fails:
![image](https://user-images.githubusercontent.com/243674/33494543-e1d07202-d6c3-11e7-9677-bc2ae72fbba9.png)

instead of a blank output.

Commits
-------

4a5a3f5 [Console][DI] Fail gracefully

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fix-silenced-errors branch Dec 3, 2017

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 3, 2017

Member

I do not appear to be able to trigger the fatal error anymore. I think you may have gotten it!

Member

weaverryan commented Dec 3, 2017

I do not appear to be able to trigger the fatal error anymore. I think you may have gotten it!

This was referenced Dec 4, 2017

fabpot added a commit that referenced this pull request Jan 23, 2018

bug #25858 [DI] Fix initialization of legacy containers by delaying i…
…nclude_once (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix initialization of legacy containers by delaying include_once

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Best reviewed ignoring whitespaces:
https://github.com/symfony/symfony/pull/25858/files?w=1

Noticed while removing a package: silencing the failing `include_once` as introduced in #25255 is not working for the `$oldContainer` in `Kernel`, and fails with a fatal error when an include succeeds but the class inside misses a parent.

Delaying the calls to `include_once` to the moment where the fresh container is actually used first,  when setting the "kernel" service, works around the situation.

Commits
-------

5e750ec [DI] Fix initialization of legacy containers by delaying include_once
@k00ni

This comment has been minimized.

Show comment
Hide comment
@k00ni

k00ni Mar 16, 2018

I experienced this bug today using Symfony 4.0.6. It occurs in certain controller actions and not when using the cache commands via terminal. But these actions call indirectly cache->clear() which seem to interfere with symfony internals?

FatalErrorException Compile Error: Container Failed opening required [...]getRouting_LoaderService.php' (include_path='.:/usr/local/lib/php') in srcDevDebugProjectContainer.php (line 295)

I was able to "fix" that for now, by removing the WebProfilerBundle resp. ProfilerPack:

  • "symfony/profiler-pack": "^1.0"
  • "symfony/web-profiler-bundle": "^4.0.6"

Afterwards i cleared the cache using rm -rf var/cache/* and it seems to work.

EDIT: See #25255 (comment) for updates to the WebProfilerBundle.

k00ni commented Mar 16, 2018

I experienced this bug today using Symfony 4.0.6. It occurs in certain controller actions and not when using the cache commands via terminal. But these actions call indirectly cache->clear() which seem to interfere with symfony internals?

FatalErrorException Compile Error: Container Failed opening required [...]getRouting_LoaderService.php' (include_path='.:/usr/local/lib/php') in srcDevDebugProjectContainer.php (line 295)

I was able to "fix" that for now, by removing the WebProfilerBundle resp. ProfilerPack:

  • "symfony/profiler-pack": "^1.0"
  • "symfony/web-profiler-bundle": "^4.0.6"

Afterwards i cleared the cache using rm -rf var/cache/* and it seems to work.

EDIT: See #25255 (comment) for updates to the WebProfilerBundle.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 16, 2018

Member

What are you calling in your controller?

Member

nicolas-grekas commented Mar 16, 2018

What are you calling in your controller?

@k00ni

This comment has been minimized.

Show comment
Hide comment
@k00ni

k00ni Mar 16, 2018

Here is part of the code:

$env = /* .. */;
$cache = new FilesystemCache('', 0, __DIR__.'/../../var/cache/'.$env);

// do some mysql operations

$cache->clear();

I expected the cache, because you mentioned it at the beginning. Furthermore, it tried to load something from the cache folder, which could not be opened.

FatalErrorException Compile Error: Container Failed opening required [...]getRouting_LoaderService.php' (include_path='.:/usr/local/lib/php') in srcDevDebugProjectContainer.php (line 295)

k00ni commented Mar 16, 2018

Here is part of the code:

$env = /* .. */;
$cache = new FilesystemCache('', 0, __DIR__.'/../../var/cache/'.$env);

// do some mysql operations

$cache->clear();

I expected the cache, because you mentioned it at the beginning. Furthermore, it tried to load something from the cache folder, which could not be opened.

FatalErrorException Compile Error: Container Failed opening required [...]getRouting_LoaderService.php' (include_path='.:/usr/local/lib/php') in srcDevDebugProjectContainer.php (line 295)

@k00ni

This comment has been minimized.

Show comment
Hide comment
@k00ni

k00ni Mar 16, 2018

Ok, i changed $cache->clear() to a solution which directly removes entries, which my app created. I re-enabled the WebProfilerBundle and tried the mentioned controller actions. And it worked. I assume, that calling $cache->clear() in a controller was the problem?

k00ni commented Mar 16, 2018

Ok, i changed $cache->clear() to a solution which directly removes entries, which my app created. I re-enabled the WebProfilerBundle and tried the mentioned controller actions. And it worked. I assume, that calling $cache->clear() in a controller was the problem?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 16, 2018

Member

I suppose. FilesystemCache is not made for clearing the full Symfony cache AFAIK.

Member

nicolas-grekas commented Mar 16, 2018

I suppose. FilesystemCache is not made for clearing the full Symfony cache AFAIK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment