Skip to content

Commit

Permalink
Do not add parent directory to file system monitoring
Browse files Browse the repository at this point in the history
`EventedFileUpdateChecker` will search the parent directory if the
specified directory does not exist.

Since `test/mailers/previews` is included in the watch target by default,
if there is no test directory (e.g. using `rspec`), the Rails root directory
will be included in the watch target.

```
$ rails new app
$ cd app
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
false
$ rm -rf test
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
true
```

This causes `node_modules` to be included in watch target. Adding parent
directories to watch target may include unexpected directories.

In order to avoid this, fixed that parents of nonexistent directories are
not added to the watch targets, instead checking that the directory
exists when checking changes.

Related to rails#32700.

[Matthew Draper & Yuji Yaginuma]
  • Loading branch information
y-yagi committed Dec 17, 2018
1 parent d57841b commit caa3cc8
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 2 deletions.
24 changes: 22 additions & 2 deletions activesupport/lib/active_support/evented_file_update_checker.rb
Expand Up @@ -52,7 +52,10 @@ def initialize(files, dirs = {}, &block)
@pid = Process.pid
@boot_mutex = Mutex.new

if (@dtw = directories_to_watch).any?
dtw = directories_to_watch
@dtw, @missing = dtw.partition(&:exist?)

if @dtw.any?
# Loading listen triggers warnings. These are originated by a legit
# usage of attr_* macros for private attributes, but adds a lot of noise
# to our test suite. Thus, we lazy load it and disable warnings locally.
Expand All @@ -75,6 +78,19 @@ def updated?
@updated.make_true
end
end

if @missing.any?(&:exist?)
@boot_mutex.synchronize do
appeared, @missing = @missing.partition(&:exist?)
shutdown!

@dtw += appeared
boot!

@updated.make_true
end
end

@updated.true?
end

Expand All @@ -96,6 +112,10 @@ def boot!
Listen.to(*@dtw, &method(:changed)).start
end

def shutdown!
Listen.stop
end

def changed(modified, added, removed)
unless updated?
@updated.make_true if (modified + added + removed).any? { |f| watching?(f) }
Expand Down Expand Up @@ -123,7 +143,7 @@ def watching?(file)
end

def directories_to_watch
dtw = (@files + @dirs.keys).map { |f| @ph.existing_parent(f) }
dtw = @files.map(&:dirname) + @dirs.keys
dtw.compact!
dtw.uniq!

Expand Down
28 changes: 28 additions & 0 deletions activesupport/test/evented_file_update_checker_test.rb
Expand Up @@ -76,6 +76,34 @@ def touch(files)

Process.wait(pid)
end

test "updated should become true when nonexistent directory is added later" do
Dir.mktmpdir do |dir|
watched_dir = File.join(dir, "app")
unwatched_dir = File.join(dir, "node_modules")
not_exist_watched_dir = File.join(dir, "test")

Dir.mkdir(watched_dir)
Dir.mkdir(unwatched_dir)

checker = new_checker([], watched_dir => ".rb", not_exist_watched_dir => ".rb") { }

FileUtils.touch(File.join(watched_dir, "a.rb"))
wait
assert_predicate checker, :updated?
assert checker.execute_if_updated

Dir.mkdir(not_exist_watched_dir)
wait
assert_predicate checker, :updated?
assert checker.execute_if_updated

FileUtils.touch(File.join(unwatched_dir, "a.rb"))
wait
assert_not_predicate checker, :updated?
assert_not checker.execute_if_updated
end
end
end

class EventedFileUpdateCheckerPathHelperTest < ActiveSupport::TestCase
Expand Down

0 comments on commit caa3cc8

Please sign in to comment.