Add raise_on_missing_assets and exceptions on missing assets

d4rky-pl commented 6 months ago

Currently WickedPDF will fail silently in most cases where assets are not available. This may not be ideal for certain scenarios where the generated PDFs must always be built properly.

This PR introduces two changes:

It includes the improvements from #1007 to avoid trying to fetch local asset over network if not needed
It introduces two new exceptions: MissingLocalAsset and MissingRemoteAsset and a new config option raise_on_missing_assets to fail when the assets are not available

d4rky-pl commented 5 months ago

@unixmonkey any chance you could approve CI on this PR? I've tested it locally but I'd love to confirm

d4rky-pl commented 5 months ago

[!CAUTION] We've discovered that running this branch in production caused a very high increase in memory use. We're still investigating why but my initial guess is that find_asset is much more costly than we believed.

I will update this PR once we figure this out

d4rky-pl commented 5 months ago

Sprockets::Railtie.build_environment(Rails.application).find_asset(path, :base_path => Rails.application.root.to_s)

This code leaks memory like crazy in sprockets 3.7.2 running in production mode (with compiled assets). It leaks on the find_asset call, likely because Sprockets::Environment is constantly recreated so nothing is ever cached properly.

Test:

# in Rails console or rake task with Rails environment
require 'get_process_mem' # @see https://github.com/zombocom/get_process_mem

def measure
  memory_before = GetProcessMem.new.mb
  result = yield
ensure
  memory_after = GetProcessMem.new.mb
  memory_diff = memory_after - memory_before
  puts "[Memory] before: #{memory_before} after: #{memory_after} diff: #{memory_diff}"

  result
end 

path = 'application.css'

puts "With fresh environment"
10.times { measure { Sprockets::Railtie.build_environment(Rails.application).find_asset(path, :base_path => Rails.application.root.to_s) } }

puts ""

puts "With reused environment"
env = Sprockets::Railtie.build_environment(Rails.application)
10.times { measure { env.find_asset(path, :base_path => Rails.application.root.to_s) } }

Result:

With fresh environment
[Memory] before: 419.7578125 after: 431.68359375 diff: 11.92578125
[Memory] before: 431.68359375 after: 442.0234375 diff: 10.33984375
[Memory] before: 442.0234375 after: 456.8515625 diff: 14.828125
[Memory] before: 456.8515625 after: 488.1796875 diff: 31.328125
[Memory] before: 488.1796875 after: 544.46875 diff: 56.2890625
[Memory] before: 544.46875 after: 547.8203125 diff: 3.3515625
[Memory] before: 547.8203125 after: 561.12109375 diff: 13.30078125
[Memory] before: 561.12109375 after: 580.53515625 diff: 19.4140625
[Memory] before: 580.53515625 after: 638.359375 diff: 57.82421875
[Memory] before: 638.359375 after: 643.2578125 diff: 4.8984375

With reused environment
[Memory] before: 643.2578125 after: 650.890625 diff: 7.6328125
[Memory] before: 650.890625 after: 650.953125 diff: 0.0625
[Memory] before: 650.953125 after: 651.015625 diff: 0.0625
[Memory] before: 651.015625 after: 651.078125 diff: 0.0625
[Memory] before: 651.078125 after: 651.140625 diff: 0.0625
[Memory] before: 651.140625 after: 651.203125 diff: 0.0625
[Memory] before: 651.203125 after: 651.265625 diff: 0.0625
[Memory] before: 651.265625 after: 651.328125 diff: 0.0625
[Memory] before: 651.328125 after: 651.390625 diff: 0.0625
[Memory] before: 651.390625 after: 651.453125 diff: 0.0625

I believe this bug is not triggered normally because in the current master the assets are fetched via URL so this line is never hit.

This issue will also affect #1007. It's already late so I'll look into implementing a proper patch tomorrow. There's still a small memory leak somewhere that doesn't happen on master but I am yet to find it.

d4rky-pl commented 5 months ago

On further investigation: fixing recreating Sprockets::Environment does indeed fix the memory leak but it still introduces around 100 MB of memory bloat. There's an easier way to grab precompiled assets from Rails by using Rails.application.assets_manifest so I've implemented that as well.

d4rky-pl commented 5 months ago

I've fixed the rubocop issue that has caused the CI run to fail and have confirmed locally that it works. Apologies for missing this!

mathieujobin commented 5 months ago

@mileszs @unixmonkey -- I would love to see if the tests runs on this at least.

d4rky-pl commented 5 months ago

I see there are conflicts already with the current master branch. Are you guys interested in this PR at all or should I close it?

unixmonkey commented 5 months ago

@d4rky-pl I am interested in this one as long as it's opt-in, and you've figured out the leak/bloat issues, which it sounds like you have.

d4rky-pl commented 5 months ago

@unixmonkey it was opt-in from the start to avoid it being a breaking change. The leak/bloat issues are already solved and we've been running this version of the library in production for almost a month now with no problems.

I've rebased the branch with latest master and fixed the broken spec, not sure why it was working fine for me locally (probably forgot to clear test/dummy)

unixmonkey commented 4 months ago

This is now released in version 2.8.0 of the Wicked PDF gem. Thank you so much for your help!

Please let me know if you have any issues!

doits commented 2 months ago

This broke production for me with a ActionView::Template::Error: no implicit conversion of nil into String in wicked_pdf-2.8.0/lib/wicked_pdf/wicked_pdf_helper/assets.rb:205:in 'join' and I traced it back to this:

I have the following code: stylesheet_link_tag wicked_pdf_asset_base64('fonts'). Notice there is no .css extension
find_asset gets path fonts
Rails.application.assets.find_asset is available in development and tests and works correctly (even wo extension)
In production is is not available, but the newly added Rails.application.assets_manifest is
The code tries to look for path inside Rails.application.assets_manifest.assets, but in my case there is only an entry for fonts.css (notice the extension). Therefore the lookup fails and I get the exception.
If I change it to stylesheet_link_tag wicked_pdf_asset_base64('fonts.css') (notice the extension), everything works again (in development and production)

So it looks like at least the branch of find_asset needs the extension whereas others (all?) don't.

Must the extension always be added then and the README should be updated?

unixmonkey commented 2 months ago

Yes, I think because Rails.application.assets_manifest.assets returns a hash where all the keys have extensions, it does appear that the extension is required.

asset_path doesn't seem to require this as it figures it out somehow in https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/asset_url_helper.rb#L243

Maybe we can do something similar here? I haven't looked into it very much yet.

joshuay03 commented 2 months ago

We also ran into a ActionView::Template::Error: no implicit conversion of nil into String error in production, but for a different reason. We have the following:

= wicked_pdf_stylesheet_link_tag "https://s3-<redacted>/application.css"

Before, #read_asset would handle precompiled_or_absolute_asset? assets first. Now, it tries to #find_asset and raises here in the File.join as it's not available in the manifest.

d4rky-pl commented 2 months ago

Ouch! I see my changes have introduced more than one bug 💔

@unixmonkey I'll try to support you on these sometime this week

d4rky-pl commented 1 month ago

Sent a PR with a fix in #1115

mileszs / wicked_pdf

Add raise_on_missing_assets and exceptions on missing assets #1094