#2738 [PATCH] Fix zero length stylesheet cache files bug in case of missing CSS files - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2738 ✓duplicate

[PATCH] Fix zero length stylesheet cache files bug in case of missing CSS files

Reported by Christos Zisopoulos | May 29th, 2009 @ 04:45 PM | in 3.0.2

There appears to be a bug with the caching version of stylesheet_link_tag that results in a all.css cache file of zero bytes (when run in production) if a missing css filename is passed as a parameter

  stylesheet_link_tag('application', 'missing_css_file', :cache => true)

What happens is that the first time around after a deploy to production, a Errno::ENOENT is raised by the code trying to join the missing file with the rest of the CSS files. However, all.css has already been opened for writing and, after the exception is raised and not handled, a zero length all.css file is left behind.

Subsequent requests serve that zero length file instead, and so we get an unexpected CSS naked day

Attached is a test case and a patch.

I've opted for ignoring the missing asset files and creating the cached assets file anyway, but it might be better to just remove the cache file altogether and raise an exception like this

  begin
    File.open(joined_asset_path, "w+") { |cache| cache.write(join_asset_file_contents(asset_paths)) }
  rescue Exception => e
    File.unlink(joined_asset_path)
    raise e
  end

What do people think? I can upload that patch instead, if it is better.

Patch against master.

fix_zero_length_stylesheet_cache_files_when_missing_css_file.diff 3.3 KB

Comments and changes to this ticket

Michael Koziarski June 10th, 2009 @ 12:47 AM
- Milestone changed from “2.x” to “2.3.4”
Michael Koziarski June 10th, 2009 @ 12:50 AM
We actually have File.atomic_write for this.
```
    #   File.atomic_write("/data/something.important", "/data/tmp") do |f|
    #     file.write("hello")
    #   end
```
the js and css caching should both probably be using that
You flagged this item as spam.
Christos Zisopoulos June 10th, 2009 @ 12:57 AM
OK. Cool.

I'll have a look at atomic_write and update the patch as required
You flagged this item as spam.
Luca Guidi June 10th, 2009 @ 10:22 AM
You shouldn't use File#readable? because it doesn't make sense on Windows, in fact that method is used to check if the file is readable by the effective user id of the current process.

I suggest File#exist? instead.
You flagged this item as spam.
Christos Zisopoulos June 12th, 2009 @ 02:26 PM
I've had a go using atomic_write and it only ensures that the asset cache file is not created. It will not take care of any exceptions raised when trying to read non-existent source asset files.

I think it boils down to these two options:

1) A missing source asset file always raises an exception and never creates a partial asset cache file.

2) Missing source asset files are ignored and the asset cache file is created only from the available source asset files.

If we went with option #1 there is the additional complication of "should the exception be raised always, whether we are caching asset files or not?"

That way you can spot the missing file(s) during development and not end up with a production-only exception.

My vote is for solution #1, but I wouldn't mind some additional input. I have the patch for solution #1 written already, just adding some tests.

@Luca Thanks for pointing that out. The definitive patch will use .exists? for Windows compatibility.
You flagged this item as spam.
Christos Zisopoulos June 12th, 2009 @ 06:04 PM
While trying to write tests for solution 1 I've discovered that a lot of the asset_tag_helper test make references to non-existent stylesheets/js files that break the tests because they only check for the return tags, not the actual files.

Writing the tests for solution 1 would require a large amount of changes to the actual test suite for asset_tag_helper

Is it worth the effort?
Michael Koziarski June 14th, 2009 @ 02:59 AM
I definitely prefer option 1, have a go at what the test changes will look like.
Christos Zisopoulos June 15th, 2009 @ 11:47 PM
- no changes were found...
- fix_zero_length_stylesheet_cache_files_when_missing_css_file_v2.diff 26.2 KB
You flagged this item as spam.
Christos Zisopoulos June 15th, 2009 @ 11:48 PM
Done. New patch attached.

An exception will be raised if a local javascript/stylesheet file included by the stylesheet_link_tag or javascript_include_tag can not be found.

When caching is enabled, we use atomic_write to ensure that the cache file is not created with zero length.

As I had to rework a large chunk of the asset_tag_helper test suit, I merged into it my related patch #2739 which improves the test suite. Feel free to close that ticket.
You flagged this item as spam.
Luca Guidi June 16th, 2009 @ 10:04 AM
Christos, in your patch, #ensure_stylesheet_sources! is declared twice, please can you clean the code?
Applied, all tests pass.
+1
You flagged this item as spam.
Luca Guidi June 16th, 2009 @ 10:09 AM
Oh sorry, I'm wrong. It isn't declared twice, but hey have very similar code..
Repository June 26th, 2009 @ 06:03 AM
- State changed from “new” to “committed”
(from [18a97a66017452dbe6cf6881c69d7a7dedc7a7bd]) Handle missing javascript/stylesheets assets by raising an exception

An exception will be raised if a local javascript/stylesheet file included
by the stylesheet_link_tag or javascript_include_tag can not be found.

When caching is enabled, we use atomic_write to ensure that the cache file
is not created with zero length.

Signed-off-by: Michael Koziarski michael@koziarski.com

[#2738 state:committed] http://github.com/rails/rails/commit/18a97a66017452dbe6cf6881c69d7a...
You flagged this item as spam.
Sam Pohlenz July 20th, 2009 @ 07:52 AM
I don't think it makes sense to have stylesheet_link_tag and javascript_include_tag assert the presence of the files when they are not being cached or concatenated.

My reason for this, is that doing so makes it impossible to have the css or javascript be generated dynamically through an action or middleware.

Am I alone in thinking that the call to ensure_stylesheet_sources! should be removed?
You flagged this item as spam.
Sam Pohlenz July 20th, 2009 @ 08:11 AM
Another situation where this breaks is in situations where a document root other than RAILS_ROOT/public is used (e.g. for multi-site apps).

A less desirable alternative would be to expose the "dumb" versions of these helpers (javascript_src_tag, stylesheet_tag) as public.
Michael Koziarski July 20th, 2009 @ 10:52 AM
@Sam,

I'd take a patch which moved that check to be solely there when :cache
or :concatenate is true
You flagged this item as spam.
Christos Zisopoulos July 20th, 2009 @ 11:10 AM
The idea of my original patch was to catch the missing asset files in development mode, so that you don't deploy to production and discover a missing asset file (either by way of the exception, or by way of the messed up CSS).

Wouldn't it make more sense to explicitly declare dynamic stylesheets as such?
```
stylesheet_link_tag('css_controller_name/dynamic.css', :dynamic => true)
```
Michael Koziarski July 20th, 2009 @ 11:15 AM
We can cover both of these cases by raising when :cache is true,
irrespective of the value of perform_caching
You flagged this item as spam.
Sam Pohlenz July 20th, 2009 @ 12:56 PM
Here's the patch.

Missing source files will be still be caught if the cache or concat options are given.
- fix_exceptions_for_missing_js_css_sources.diff 5.8 KB
Michael Koziarski July 20th, 2009 @ 11:15 PM
- Milestone cleared.
- State changed from “committed” to “open”
Opening and targetting 3.0
josh September 3rd, 2009 @ 07:58 PM
This bit us too. We had a nonexistent javascript file that was generated outside of javascript_include_tag.
Rizwan Reza January 17th, 2010 @ 02:21 AM
- Tag changed from “action_view, assets, asset_tag_helper, caching, master, zero” to “3.0, action_view, assets, asset_tag_helper, caching, master, zero”
- Assigned user set to “Michael Koziarski”
Patch doesn't apply anymore.
Michael Koziarski January 17th, 2010 @ 02:30 AM
- Assigned user changed from “Michael Koziarski” to “josh”
If it bit josh, he can shepherd this one in.
Rizwan Reza January 17th, 2010 @ 02:48 AM
This has been applied already, in a modified way. Can close this ticket.
Michael Koziarski January 17th, 2010 @ 02:50 AM
- State changed from “open” to “duplicate”
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “Low”
[bulk edit]