Skip to content
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

[5.8] Replace contents of service manifest atomically #28973

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

wmouwen
Copy link
Contributor

@wmouwen wmouwen commented Jun 27, 2019

It is possible for concurrent requests to try and write the services manifest (bootstrap/cache/services.php) at the same time. This can occur whenever the file is not present (e.g. after php artisan clear-compiled) or when the set of providers changes (after code changes).

Each request will first read the manifest file or detect its absence, take some time to generate the contents of a new manifest, and lastly try to write the manifest. This write action will be done without obtaining a file lock first.

$this->files->put(
$this->manifestPath, '<?php return '.var_export($manifest, true).';'
);

We have encountered broken service manifest files in our installations multiple times in the last few days. Once it breaks, both HTTP requests and Artisan requests will fail. Manual deletion of the file is needed.

Example of broken file:

<?php return array (
  'providers' =>
  array (
    0 => 'Laravel\\Tinker\\TinkerServiceProvider',
    1 => 'Carbon\\Laravel\\ServiceProvider',
    2 => 'Company\\Providers\\RouteServiceProvider',
  ),
  'eager' =>
  array (
    0 => 'Carbon\\Laravel\\ServiceProvider',
    1 => 'Company\\Providers\\RouteServiceProvider',
  ),
  'deferred' =>
  array (
    'command.tinker' => 'Laravel\\Tinker\\TinkerServiceProvider',
  ),
  'when' =>
  array (
    'Laravel\\Tinker\\TinkerServiceProvider' =>
    array (
    ),
  ),
);seServiceProvider',
    9 => 'Company\\Providers\\SessionServiceProvider',
    10 => 'Company\\Providers\\ViewServiceProvider',
    11 => 'Company\\Providers\\AppServiceProvider',
    12 => 'Company\\Providers\\AuthServiceProvider',
    13 => 'Company\\Providers\\ViewServiceProvider',
    14 => 'Company\\Providers\\RouteServiceProvider',
  ),
  'eager' =>
  array (
    0 => 'Illuminate\\Filesystem\\FilesystemServiceProvider',
    1 => 'Carbon\\Laravel\\ServiceProvider',
    2 => 'Company\\Providers\\DatabaseServiceProvider',
    3 => 'Company\\Providers\\SessionServiceProvider',
    4 => 'Company\\Providers\\ViewServiceProvider',
    5 => 'Company\\Providers\\AppServiceProvider',
    6 => 'Company\\Providers\\AuthServiceProvider',
    7 => 'Company\\Providers\\ViewServiceProvider',
    8 => 'Company\\Providers\\RouteServiceProvider',
  ),

This pull request will alter the behavior of writing the manifest file so it uses atomic actions. Concurrent writing will no longer occur.

Please note that this bug has been around since version 5.2.0

@wmouwen wmouwen force-pushed the atomic_replace_service_manifest branch from 11b0ef4 to 563e7d4 Compare June 27, 2019 09:06
@wmouwen wmouwen changed the title Replace contents of service manifest atomically [5.8] Replace contents of service manifest atomically Jun 27, 2019
@taylorotwell taylorotwell merged commit 8dcd047 into laravel:5.8 Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants