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

Paperclip styles syntax #12812

Closed
wants to merge 2 commits into from
Closed

Paperclip styles syntax #12812

wants to merge 2 commits into from

Conversation

hugogameiro
Copy link
Contributor

I was running into an issue where Sidekiq was getting jobs stuck indefinitely when attempting to import a new account header. Doing the manual regenerate via the web interface also failed.

After digging around, I found that it only happened on header files using animated gifs that needed conversion/rescaling and only on instances using Docker.

What worked for me was changing styles to style on paperclip calls as mentioned here: thoughtbot/paperclip#1405 (comment)

This puzzles me because PaperClip is deprecated and according to that user (in 2016) style was the new syntax. Also, can't understand why it only affects Dockers installs.

So, I don't know if this will break for others (not using Docker) and it would be great if someone tested it as I don't have a non Docker test setup.

Also, https://github.com/tootsuite/mastodon/blob/master/app/models/preview_card.rb also uses styles but haven't tested it and didn't what to PR before I do.

@Gargron
Copy link
Member

Gargron commented Jan 9, 2020

This is an odd case of multiple people being wrong, I think. :style is not a real option. It doesn't exist. The real option is :styles. Because you are removing all transformations by moving them to a key that isn't read by anything, transformations that caused errors previously do not cause them anymore.

@hugogameiro
Copy link
Contributor Author

Yes, you are correct. I am still looking into this. All I did was bypass the styles, it solves the problem but it's a work around and not the real issue. I will continue to dig deeper into this, tested this just now but no luck. It really looks like something to do with Docker but can't find the source.

@Gargron
Copy link
Member

Gargron commented Jan 10, 2020

You said the jobs were getting stuck. Were you getting Paperclip::Errors::NotIdentifiedByImageMagickError errors at all?

@hugogameiro
Copy link
Contributor Author

hugogameiro commented Jan 10, 2020

No error. Even Rails debug didn't show a thing. It just reaches this point and freezes:

Command :: convert '/tmp/8d777f385d3dfec8815d20f7496026dc20200109-11-5ge97x' -coalesce -auto-orient -resize "1244x603>" -layers "optimize" -strip '/tmp/22acefd3a132a94c9e92a7ec69aea57d20200109-11-1ceav72'

If multiple jobs get stuck like this, federation just stops indefinitely. Jobs don't timeout when they are stuck like this.

It happened yesterday to a couple of instances I host. All because a single user from a MissKey instance had an animated gif header with dimensions that needed resize and posts from that user were boosted. Every boost got a different job and every job froze attempting to fetch the header. This is what caused me to rush out with a fix because I couldn't have instances that stop to federate.

Initially I thought it was the STDOUT overflow error that caused previous issues #9368 and #12088 but adding loglevel on the command did not solve it.

@hugogameiro
Copy link
Contributor Author

hugogameiro commented Jan 10, 2020

So, what I found:

  • only specific animated gifs cause this issue
  • both avatar and header files that need resizing are affected, both local upload and federated import
  • both Docker and non-Docker are affected but on non-Docker it just ends up timing-out (probably not enough resources are available to process in time) and on Docker it hangs (at least on my setup)

The specific gif that caused this issue had 200 frames with 1280x620 and 578 KB. It takes MS over 20 seconds to process this image and by the limits in place (2MB no frame limit) it could be 3 times larger.

@hugogameiro
Copy link
Contributor Author

After looking more into this, I believe that animated gifs will always be tricky to convert/resize. I couldn't find a simple solution. Currently the header/avatar is being handled by Paperclip/ImageMagick and like ImageMagick resize animation page says:

If it is at all possible, DO NOT RESIZE.
You can for example Canvas or Viewport Crop your animation to just trim it down to fit in the space you need it for.
Or you can generate the GIF animation at the right size in the first place.
Neither technique is typically the best option, but if you can, consider it, as it will save you a lot of problems and hair pulling.

I am currently testing just setting a timeout for ImageMagick via environment variables MAGICK_TIME_LIMIT to see if that helps to avoid freezing Sidekiq jobs. Like mentioned on PaperClip docs. It won't solve the problem but hopefully it avoids halting Sidekiq.

@hugogameiro hugogameiro deleted the paperclip_styles branch January 11, 2020 20:36
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