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

Projects
None yet
2 participants
@wmouwen
Copy link
Contributor

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 wmouwen:atomic_replace_service_manifest branch from 11b0ef4 to 563e7d4 Jun 27, 2019

@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

1 check passed

continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.