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

Raise error when two assets produce same path #530

Merged
merged 4 commits into from Mar 12, 2018

Conversation

schneems
Copy link
Member

When trying to generate assets we can have multiple files that generate the same end path. For example .css and .css.erb and .scss all generate a file with .css extension. When linking assets (telling sprockets to generate a new file) we can check for the condition that the same asset logical path has already been generated when that happens we should get an error like this:

Sprockets::DoubleLinkError: Multiple files with the same output path cannot be linked ("stylesheets/a.css")
In "/Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/link_directory_manifest.js" these files were linked:
  - /Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/stylesheets/a.css
  - /Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/stylesheets/a.css.erb

This does not handle the case where individual assets are directly linked (versus using link_directory).

When trying to generate assets we can have multiple files that generate the same end path. For example `.css` and `.css.erb` and `.scss` all generate a file with `.css` extension. When linking assets (telling sprockets to generate a new file) we can check for the condition that the same asset logical path has already been generated when that happens we should get an error like this:

```
Sprockets::DoubleLinkError: Multiple files with the same output path cannot be linked ("stylesheets/a.css")
In "/Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/link_directory_manifest.js" these files were linked:
  - /Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/stylesheets/a.css
  - /Users/rschneeman/Documents/projects/sprockets/test/fixtures/double/stylesheets/a.css.erb
```

This does not handle the case where individual assets are directly linked (versus using link_directory).
@schneems
Copy link
Member Author

Failure on windows:

  1) Error:
TestManifest#test_"double rendering with link_directory raises an error":
Errno::ENOTEMPTY: Directory not empty @ dir_s_rmdir - C:/Users/appveyor/AppData/Local/Temp/1/d20171130-2988-1006cuf
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1444:in `rmdir'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1444:in `block in remove_dir1'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1458:in `platform_support'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1443:in `remove_dir1'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1436:in `remove'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:778:in `block in remove_entry'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1493:in `ensure in postorder_traverse'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:1493:in `postorder_traverse'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:776:in `remove_entry'
    C:/Ruby22/lib/ruby/2.2.0/fileutils.rb:700:in `remove_entry_secure'
    C:/projects/sprockets/test/test_manifest.rb:18:in `teardown'
883 runs, 3929 assertions, 0 failures, 1 errors, 8 skips

No clue why it only would happen on this method

@schneems
Copy link
Member Author

schneems commented Dec 7, 2017

Turns out this isn't a windows bug, it's a threading bug. It just happens to show up more often on windows:

Here's what's happening. There is some code that calls find_all_linked_assets which is where our new error is coming from

environment.find_all_linked_assets(path) do |asset|
  yield asset
end

You'll notice that there's a yield. What block is being executed? It's this one:

find(*args) do |asset|
mtime = Time.now.iso8601
files[asset.digest_path] = {
'logical_path' => asset.logical_path,
'mtime' => mtime,
'size' => asset.bytesize,
'digest' => asset.hexdigest,
# Deprecated: Remove beta integrity attribute in next release.
# Callers should DigestUtils.hexdigest_integrity_uri to compute the
# digest themselves.
'integrity' => DigestUtils.hexdigest_integrity_uri(asset.hexdigest)
}
assets[asset.logical_path] = asset.digest_path
filenames << asset.filename
promise = nil
exporters_for_asset(asset) do |exporter|
next if exporter.skip?(logger)
if promise.nil?
promise = Concurrent::Promise.new(executor: executor) { exporter.call }
concurrent_exporters << promise.execute
else
concurrent_exporters << promise.then { exporter.call }
end
end
end

You'll notice this code is building promises and then waiting for them:

    # ...
    if promise.nil?
      promise = Concurrent::Promise.new(executor: executor) { exporter.call }
      concurrent_exporters << promise.execute
    else
      concurrent_exporters << promise.then { exporter.call }
    end
  end
end

# make sure all exporters have finished before returning the main thread
concurrent_exporters.each(&:wait!)
save

Unfortunately, the block ends before concurrent_exporters.each(&:wait!) gets called.

So what is happening is environment.find_all_linked_assets(path) do |asset| will yield a.css which enqueues background threads to write contents to disk. Then it will try to yield a.css.erb but before it can it will raise the DoubleRender error. When this happens the promises that are getting processed in the background are never wait!-ed and the program continues from where the exception was rescued. This happens in our test case will try to clean up, yet files are still open and being written to. The operating system tries to delete the directory but there are open file descriptors so it fails, with this somewhat odd error message.

Here's what's happening. There is some code that calls `find_all_linked_assets` which is where our new error is coming from

```
environment.find_all_linked_assets(path) do |asset|
  yield asset
end
```

You'll notice that there's a yield. What block is being executed? It's this one: https://github.com/rails/sprockets/blob/d31aabc023bb58035bb1e2e03be6e0d4e88e1539/lib/sprockets/manifest.rb#L168-L196

You'll notice this code is building promises and then waiting for them:

```ruby
    # ...
    if promise.nil?
      promise = Concurrent::Promise.new(executor: executor) { exporter.call }
      concurrent_exporters << promise.execute
    else
      concurrent_exporters << promise.then { exporter.call }
    end
  end
end

# make sure all exporters have finished before returning the main thread
concurrent_exporters.each(&:wait!)
save
```

Unfortunately, the block ends before `concurrent_exporters.each(&:wait!)` gets called. 

So what is happening is `environment.find_all_linked_assets(path) do |asset|` will yield `a.css` which enqueues background threads to write contents to disk. Then it will try to yield `a.css.erb` but before it can it will raise the `DoubleRender` error. When this happens the promises that are getting processed in the background are never `wait!`-ed and the program continues from where the exception was rescued. This happens in our test case will try to clean up, yet files are still open and being written to. The operating system tries to delete the directory but there are open file descriptors so it fails, with this somewhat odd error message.
@schneems schneems merged commit 983a791 into master Mar 12, 2018
@jeremy jeremy deleted the schneems/double-render branch March 12, 2018 23:10
@Fudoshiki
Copy link
Contributor

Fudoshiki commented Mar 13, 2018

Error 983a791#commitcomment-28065318

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.

None yet

2 participants