Add raise_on_missing_assets and exceptions on missing assets

[d4rky-pl]

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 Try to fetch asset from filesystem first before potentially going over the network #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]

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

[d4rky-pl]

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]

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]

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]

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]

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

[d4rky-pl]

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]

@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]

@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]

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]

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]

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]

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]

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]

Sent a PR with a fix in #1115

[wandji20]

This may come late but I also get an error ActionView::Template::Error (no implicit conversion of nil into String) in production when including a stylesheet compiled via webpacker.

I am positive Rails.application.assets_manifest.assets[path] in find_asset will return nil if the asset is not compiled by sprockets (in my case Webpacker).
Can anyone tell me more about how the gem handles webpacker compiled assets in production.

[d4rky-pl]

@wandji20 can you confirm you're running into this issue also with the changes from #1115 ?

[wandji20]

Thanks @d4rky-pl
I introduced the mentioned change and now the error is gone 😄

Just one more query!
How does the find_asset method handle Webpacker-compiled files?
I had expected to see some use of Webpacker.manifest.lookup(path) but not sure if I am missing something here.