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

bugfix for thumbnails TmpStore generation #14615

Closed

Conversation

bitbirddev
Copy link
Contributor

ok so this explanation is going to be a hard one.

to reproduce this issue:

create a twig template with a {{ pimcore_image("image") }}-tag. Open the template in the backend and add an image to the editable. right-click on the image and select a specific area of the image. save the document.

if you open the document in the frontend, the image is display and behind the scenes some entries are created in the TmpStore (database table tmp_store) and one entry is created in the ThumbnailCache (database table assets_image_thumbnail_cache)

everything is alright so far. now right click on the image and open it in a new tab. Refresh the table assets_image_thumbnail_cache in the database and you'll see that the entry of your image is now gone.

If you reload the document in the frontend you would assume that the entry is created again - but it isn't because the thumbnail-file still exists in the storage and therefore no TmpStore-Entry is created. if you host your thumbnails on for example s3 than the frontend_prefix for your assets is never attached because of the missing entry in assets_image_thumbnail_cache.

now this "open in new tab" thing is just a way to reproduce this issue. in a real world example this happens when 2 users visit one document with image-thumbnails that are still being processed (deferred). the second visit of the site wipes all those entries in assets_image_thumbnail_cache and now your frontend-prefixes are missing for those thumbnails.

to fix this issue we have to not only remove the thumbnail from the cache but also from the storage. thats exactly what this PR does.

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@mattamon
Copy link
Contributor

Hey @bitbirddev,

I tried to reproduce it but the entries in the table assets_image_thumbnail_cache are not cleared for me.
I did everything according to your description and then after I opened the document in the frontend and right-clicked the image to open it in a new tab, the entries are still in the table.

Is there another way of reproducing it? Maybe you can show me a video of the problem?

@bitbirddev
Copy link
Contributor Author

@mattamon i hope this helps: Link to Video

@bitbirddev
Copy link
Contributor Author

@mattamon i hope this helps: Link to Video

please also keep in mind that you have to clear temporary files before trying the steps in the video. after doing the steps in the video, the file "wiss Begleitung.avif" was still in the thumbnails-folder but the entry in assets_image_thumbnail_cache was gone.

@mattamon
Copy link
Contributor

@bitbirddev thank you for the video and the hints! Unfortunately, I am still not able to reproduce it.
I'm going to talk to my colleagues, maybe they can reproduce the problem.

@bitbirddev
Copy link
Contributor Author

@mattamon do you have frontend_prefix and thumbnail_cache enabled?

@bitbirddev
Copy link
Contributor Author

bitbirddev commented Mar 14, 2023

@mattamon a little more detailed. hope that helps!

to reproduce

  • create a pimcore document with image editable {{ pimcore_image("test") }}
  • select some image for the editable
  • right click on the image => select specific area of image
  • save + publish document
  • clear all temporary files
  • truncate tables tmp_store and assets_image_thumbnail_cache
  • open document in frontend

what happens behind the curtains

  • twig generates a srcset for the image, in my case 6 different links for 6 different formats and fills the table tmp_store with 6 auto generated thumbnail configs

  • your browser decides which thumbnail to load

  • in my case its the <filename>@2x.avif-Version of the Thumbnail

  • the link in my case is /header/166/image-thumb__166___auto_da5460e6a5e4f5904ff70b047fda6823/wiss%20Begleitung@2x.avif

  • PublicServicesController.php is responsible for delivering the thumbnails

  • the thumbnail config gets loaded from TmpStore and deleted afterwards

  • after creating the thumbnail, an entry in assets_image_thumbnail_cache is created

  • you should now see the image in your browser

  • if you now right click on the image and open it in a new tab the whole process begins again as the src of the image is still the same (/header/166/image-thumb__166___auto_da5460e6a5e4f5904ff70b047fda6823/wiss%20Begleitung@2x.avif)

  • this time, there is no matching entry in the TmpStore because it was deleted earlier

  • because of the missing entry in TmpStore the entry in assets_image_thumbnail_cache gets deleted

  • now we do not have a matching entry in tmp_store and we also do not have an entry in assets_image_thumbnail_cache, but we still have the image thumbnail file (generated earlier) on our disk

  • if you now reload the document the image is displayed to you but the entries in tmp_store wont change and the assets_image_thumbnail_cache table is still empty

  • this is because $deferred is always true now what prevents pimcore from writing a new entry in the assets_image_thumbnail_cache table

  • the link to your image will now always be a deferred one. since the ['frontend_prefixes']['thumbnail_deferred'] is 'null' in most cases the link to the image just points back to the PublicServicesController

now what does it mean?

imagine you have a document displaying multiple images. for example a news listing with preview images. now you push 5 new entries. if 2 or more users load the page in parallel chances are high that one of the requests will delete the entry of a thumbnail in assets_image_thumbnail_cache while the processor is working. now for this thumbnail you'll never see your link with frontend_prefixes ever again.

i was also able to reproduce this with just hitting the refresh button while the images are still being loaded.

@mattamon
Copy link
Contributor

@bitbirddev Thank you so much for your effort! I was finally able reproduce the error.
It opened it in different browsers and now the image is gone.
It also depends on what picture your browser wants to display!
I also tried the exact steps with your patch applied, but it does not seem to fix it for me. Need to investigate this a little more.

@bitbirddev
Copy link
Contributor Author

bitbirddev commented Mar 15, 2023

@mattamon you're welcome! glad to hear you could reproduce the bug. i'm using my fix for some month now and never had a problem with it since then. did you clear out everything (including tmp files) after applying the patch? you can see in the in assets_image_thumbnail_cache which thumbnail your browser tried to load, then you just inspect the image and open the corresponding entry from the srcset in a new tab. after that the entry in assets_image_thumbnail_cache is gone.

i knew from the beginning that this would be a bug hard to describe/reproduce. now you can imagine how much time it took me to find out why some images aren't loaded from the cdn after some time in production :D

@mattamon
Copy link
Contributor

mattamon commented Mar 15, 2023

@bitbirddev Yes I did the exact same steps as before.
After clearing the tmp files and the tables I open it up in firefox and instantly after in Chrome. It is broken in Chrome and if I try to load it in firefox afterwards, firefox is also broken. Maybe you can check this on your side too?

@bitbirddev
Copy link
Contributor Author

@mattamon just checked it and yeah it makes sense so far.

with my fix

  • first load of page, thumbnail gets generated, assets_image_thumbnail_cache gets filled with an entry
  • open the same image link again => PublicServicesController will remove entry in assets_image_thumbnail_cache and the thumbnail file on disk (now you can open that url as often as you want, you'll just see the error thumb
  • when you open the document (with the {{ pimcore_image-tag }}) again it'll start over, you'll see the thumbnail and an entry in assets_image_thumbnail_cache will be created
  • now if you do not open thumbnails in new tabs or anything and just make another reload your images get loaded with "frontend_prefixes" because now {{ pimcore_image }} knows about the entry in assets_image_thumbnail_cache

in easy words: on the first pageload all your image-links point to the PublicServicesController which creates Thumbnails and fills assets_image_thumbnail_cache. the second pageload already knows about your thumbnails and therefore adds frontend_prefixes

keep in mind that not all thumbs of the img srcset are generated. only the versions your browser requests.

without the fix

entry in assets_image_thumbnail_cache could be deleted due to parallel requests and will not be created again afterwards. thumbnails where this happens will never be loaded via cdn

@robertSt7 robertSt7 modified the milestones: 10.5.19, 10.5.20 Mar 15, 2023
@bitbirddev
Copy link
Contributor Author

@mattamon any updates on this?

@mattamon
Copy link
Contributor

@bitbirddev, sorry not yet. We have to check this a little more. I'm going to get back to you once we finished reviewing it!

@dvesh3 dvesh3 self-assigned this Mar 20, 2023
@dvesh3 dvesh3 removed this from the 10.5.20 milestone Mar 27, 2023
@mattamon
Copy link
Contributor

@bitbirddev sorry we still need to have a deeper look into this. We are currently finalizing some other things, but we'll come back to you.

@dvesh3
Copy link
Contributor

dvesh3 commented Jun 21, 2023

Hi @bitbirddev please rebase your PR on 10.6 branch since 10.5 branch will be removed soon after tagging the last 10.5 release. Sorry for the inconvenience!

@dvesh3 dvesh3 added the Backlog label Jun 21, 2023
@brusch brusch changed the base branch from 10.5 to 10.6 June 27, 2023 08:22
@dvesh3 dvesh3 removed the Backlog label Jun 27, 2023
@dvesh3
Copy link
Contributor

dvesh3 commented Jul 10, 2023

@bitbirddev could you please check if this fixes the issue for you? thanks!

@dvesh3
Copy link
Contributor

dvesh3 commented Jul 11, 2023

Closed in favor of #15351

@dvesh3 dvesh3 closed this Jul 11, 2023
@bitbirddev
Copy link
Contributor Author

@bitbirddev could you please check if this fixes the issue for you? thanks!

@dvesh3 @mattamon i'm sorry for my late response. i just reproduced this issue with pimcore v10.6.7 so i don't think this issue is not fixed with #15351 . The Fix has to be in PublicServicesController - this file wasn't even changed in #15351 . Can you reopen this issue?

@bitbirddev
Copy link
Contributor Author

Hi @dvesh3 @mattamon This Issue is not fixed and still exists in v11.1.4.
Its really annoying finding dead images or images that won't load via CDN all over the site.

Even more frustrating is that with srcsets i can't really see which images are dead without testing the whole srcset of the image...

Can you guys please reopen the issue?

@bitbirddev
Copy link
Contributor Author

additionally: in combination with the full page cache, delivering assets via cdn wont work at all since the first pageload (without frontend_prefixes added to the urls) is the one to be cached...

@fashxp fashxp reopened this Jan 10, 2024
Copy link

sonarcloud bot commented Jan 10, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@bitbirddev
Copy link
Contributor Author

find a more detailed bug report here #16486

@brusch
Copy link
Member

brusch commented Feb 7, 2024

Obsolete due #16529

@brusch brusch closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants