statamic / cms

The core Laravel CMS Composer package
https://statamic.com
Other
3.59k stars 492 forks source link

CP Thumbnails timing out with 524 - High CPU #7096

Open ryatkins opened 1 year ago

ryatkins commented 1 year ago

Bug description

CP Thumbnails keep timing out upon loading of an entry and they don't look to be cached.

I have run php artisan statamic:assets:generate-presets and it has completed without errors.

image

How to reproduce

Load an entry with images.

Images are stored on Digital Ocean Spaces.

Thumbnails are being stored on the filesystem.

Logs

No response

Environment

Environment
Laravel Version: 9.37.0
PHP Version: 8.1.12
Composer Version: 2.4.4
Environment: production
Debug Mode: ENABLED
URL: staging...
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: CACHED
Views: CACHED

Drivers
Broadcasting: log
Cache: statamic
Database: mysql
Logs: stack / single
Mail: array
Queue: sync
Session: file

Statamic
Addons: 5
Antlers: regex
Version: 3.3.49 PRO

Statamic Addons
aerni/imagekit: 2.2.0
doublethreedigital/duplicator: 2.3.2
jonassiewertsen/statamic-livewire: 2.9.0
statamic/seo-pro: 3.1.0
xndbogdan/statamic-bard-text-color: 1.0.2

Installation

Existing Laravel app

Antlers Parser

regex (default)

Additional details

This started happening after update from 3.1 to 3.3.

jasonvarga commented 1 year ago

Doesn't look like just thumbnails. Even the relationship fieldtype ajax requests are doing it.

ryatkins commented 1 year ago

I removed all the relationships and locally it seemed to help, but on my staging server it still can't load all the images.

image

I'm not even able to Log the asset it is requesting, I only get the Out of memory error below.

    /**
     * Display the thumbnail.
     *
     * @param  string  $asset
     * @param  string  $size
     * @param  string  $orientation
     * @return \Illuminate\Http\Response
     */
    public function show($asset, $size = null, $orientation = null)
    {
        $this->size = $size;
        $this->orientation = $orientation;
        $this->asset = $this->asset($asset);

        Log::alert('Asset: '.$this->asset.' | Size: '.$this->size.' | Orientation: '.$this->orientation);
[2022-11-21 21:28:39] production.ERROR: Out of memory (allocated 6291456) (tried to allocate 53126944 bytes) {"userId":"0c723e06-8644-404c-b122-6e6b06959036","exception":"[object] (Symfony\\Component\\ErrorHandler\\Error\\FatalError(code: 0): Out of memory (allocated 6291456) (tried to allocate 53126944 bytes) at /home/forge/my.site.com/vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php:79)
[stacktrace]
#0 {main}
ryatkins commented 1 year ago

I've reduced the entry down from 25 to 4 images. The entry and images will load, but each image takes 10 seconds each to generate. So 25 images trying to do this all at once is exceeding my server's memory and pegging my CPU to 100%.

Why are these images having to generate every time and not using the cache? Especially after running generate-presets which takes my 1500 image assets and makes a total of 4000+ images on my server.

FYI - some settings

STATAMIC_STACHE_WATCHER=false
SAVE_CACHED_IMAGES=false
CACHING_STRATEGY=full
image
ryatkins commented 1 year ago

With SAVE_CACHED_IMAGES=true I get the following:

League\Flysystem\UnableToReadFile: Unable to read file from location: containers/assets/mysite/products/MYIMAGE_1.png/1325e638f29d94e32146dc14b5a66472.png. in file /home/forge/staging.mysite.com/vendor/league/flysystem/src/UnableToReadFile.php on line 24

File does exist, but was created in /storage and not /public/img by generate-presets:

forge@my-site:~/staging.mysite.com/storage/statamic/glide$ 
ls containers/assets/mysite/products/MYIMAGE_.png/1325e638f29d94e32146dc14b5a66472.png

containers/assets/mysite/products/MYIMAGE_1.png/1325e638f29d94e32146dc14b5a66472.png

Linking the files in /storage/statamic/glide via ln -s in /public/img does allow them to be served, but a single image render is still spiking the CPU to 88%.

jasonvarga commented 1 year ago

Please also provide your asset container yaml file (content/assets/container.yaml) and config/filesystems.php.

ryatkins commented 1 year ago

content/assets/assets.yaml (I don't have a container.yaml)

title: Assets
disk: spaces
allow_uploads: true
allow_downloading: true
allow_renaming: true
allow_moving: true
create_folders: true

config/filesystems.php

<?php

return [

    /*
    |--------------------------------------------------------------------------
    | Default Filesystem Disk
    |--------------------------------------------------------------------------
    |
    | Here you may specify the default filesystem disk that should be used
    | by the framework. The "local" disk, as well as a variety of cloud
    | based disks are available to your application. Just store away!
    |
    */

    'default' => env('FILESYSTEM_DISK', 'local'),

    'cloud' => env('FILESYSTEM_CLOUD', 's3'),

    /*
    |--------------------------------------------------------------------------
    | Filesystem Disks
    |--------------------------------------------------------------------------
    |
    | Here you may configure as many filesystem "disks" as you wish, and you
    | may even configure multiple disks of the same driver. Defaults have
    | been set up for each driver as an example of the required values.
    |
    | Supported Drivers: "local", "ftp", "sftp", "s3"
    |
    */

    'disks' => [
        'content' => [
            'driver' => 'local',
            'root' => base_path('content'),
        ],

        'local' => [
            'driver' => 'local',
            'root' => storage_path('app'),
            'throw' => false,
        ],

        'public' => [
            'driver' => 'local',
            'root' => storage_path('app/public'),
            'url' => env('APP_URL').'/storage',
            'visibility' => 'public',
            'throw' => false,
        ],

        's3' => [
            'driver' => 's3',
            'key' => env('AWS_ACCESS_KEY_ID'),
            'secret' => env('AWS_SECRET_ACCESS_KEY'),
            'region' => env('AWS_DEFAULT_REGION'),
            'bucket' => env('AWS_BUCKET'),
            'url' => env('AWS_URL'),
            'endpoint' => env('AWS_ENDPOINT'),
            'cache' => true,
            'disable_asserts' => true,
            'use_path_style_endpoint' => env('AWS_USE_PATH_STYLE_ENDPOINT', false),
            'throw' => false,
        ],

        'assets' => [
            'driver' => 'local',
            'root' => public_path('assets'),
            'url' => '/assets',
            'visibility' => 'public',
        ],

        'favicons' => [
            'driver' => 'local',
            'root' => public_path('favicons'),
            'url' => '/favicons',
            'visibility' => 'public',
        ],

        'spaces' => [
            'driver' => 's3',
            'key' => env('DO_SPACE_KEY'),
            'secret' => env('DO_SPACES_SECRET'),
            'region' => env('DO_SPACES_REGION'),
            'bucket' => env('DO_SPACES_BUCKET'),
            'endpoint' => env('DO_SPACES_ENDPOINT'),
            'url' => '/',
            'visibility' => 'public',
            'cache' => true,
            'disable_asserts' => true,
        ],

        'tmp-for-tests' => [
            'driver' => 'local',
            'root' => storage_path('app/livewire-tmp'),
        ],

    ],

    /*
    |--------------------------------------------------------------------------
    | Symbolic Links
    |--------------------------------------------------------------------------
    |
    | Here you may configure the symbolic links that will be created when the
    | `storage:link` Artisan command is executed. The array keys should be
    | the locations of the links and the values should be their targets.
    |
    */

    'links' => [
        public_path('storage') => storage_path('app/public'),
    ],

];
jasonvarga commented 1 year ago

content/assets/assets.yaml (I don't have a container.yaml)

Yeah that's good, I meant like the whatever-your-container-is.yaml 😄 Thanks!

ryatkins commented 1 year ago

Doing some speed testing on my local machine for a single image:

Accessing the image directly via URL: 5ms

Accessing the image through the ThumbnailController with SAVE_IMAGE_CACHE true or false 450ms

Using PHP's readfile() to stream to the browser: 450ms

How do we get Statamic to just serve up a URL instead of having to load the image into memory and stream to the browser?

stuartcusackie commented 1 year ago

I have also ran php artisan statamic:assets:generate-presets, but the CP asset container is still quite slow at loading the 30x30 thumbs. It seems to be manipulating each one at a time on the first load, and I'm seeing similar network usage in the console as the original post - 2 seconds for each image to be generated.

Not sure if this is relevant but the urls contain /cp/thumbnails/: image Shouldn't they be using the /img/ folder in order to hit the cache?

ryatkins commented 1 year ago

@stuartcusackie - from what I can tell, there is no way in Statamic's CP to directly hit the actual URL of a thumbnail. Everything goes through ThumbnailController via /cp/thumbnails and from my testing, there is at least a 450-500ms penalty for this. I believe the reason for the penalty is that PHP is reading each image into memory and then streaming to the browser.

@jasonvarga - would you be open to a PR that allows users via a setting to directly reach the actual image url? I have a PR already started.

jasonvarga commented 1 year ago

If you have the PR already started then go for it.

stuartcusackie commented 1 year ago

Just out of curiosity... Why would anyone not want this change and why is a setting required? Shouldn't it just be the default? I don't understand why these images aren't treated the same as any other glide images 😕

ryatkins commented 1 year ago

PR https://github.com/statamic/cms/pull/7135 submitted.

jasonvarga commented 1 year ago

Just out of curiosity... [...] why these images aren't treated the same as any other glide images

Good question! I don't know the answer right now but it looks like the thumbnail route and controller were brought over from v2. It may be possible that we can rework it to not be necessary anymore.

jasonvarga commented 1 year ago

Looking back at the history, I believe the reason for a dedicated controller was that if we were just use glide, and you had the "cached" setting enabled... if you hit the asset listing for the first time it would generate thumbnails for everything in the listing, potentially resulting in either a long request or a timeout.

By using a dedicated thumbnail route, the listing could load fast, then each image request would take as long as needed.

stuartcusackie commented 1 year ago

ah yes, I see what you mean. You could time out your control panel quite easily if the assets aren't pre-manipulated and the glide cache is enabled. Maybe some kind of placeholder image with JS lazy loading could be a workaround.

ryatkins commented 1 year ago

The approach I took in PR #7135 is definitely a more advanced approach as the images may not exist. But a lot faster way to retrieve all your images and not tax the server.