Skip to content

Commit

Permalink
EventedFileUpdateChecker boots once per process
Browse files Browse the repository at this point in the history
We need one file checker booted per process as talked about in rails#24990. Before we do a check to see if any updates have been registered by the listener we first check to make sure that the current process has booted a listener.

We are intentionally not starting a listener when the checker is created. This way we can avoid rails#25259 in which puma warns of multiple threads created before fork. As written the listener for each process will be invoked by the `ActionDispatch::Executor` middleware when the `updated?` method is called. This is the first middleware on the stack and will be invoked before application code is read into memory.

The downside of this approach is that the API is a little less obvious. I.e. that you have to call `updated?` to get the listener to start is not intuitive. We could make `boot!` not private if we want to make the API a little nicer. Alternatively we could boot when the checker is initialized however this reintroduces the puma threads warning, and also means that in cases of `rails server` or when using `preload!` that we have extra threads notifying of changes on a process that we don't care about.

[close rails#24990] [close rails#25259]
  • Loading branch information
schneems committed Jun 6, 2016
1 parent 3fc5577 commit 7bd4199
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
15 changes: 10 additions & 5 deletions activesupport/lib/active_support/evented_file_update_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ def initialize(files, dirs = {}, &block)
@dirs[@ph.xpath(dir)] = Array(exts).map { |ext| @ph.normalize_extension(ext) }
end

@block = block
@updated = Concurrent::AtomicBoolean.new(false)
@lcsp = @ph.longest_common_subpath(@dirs.keys)
@block = block
@updated = Concurrent::AtomicBoolean.new(false)
@lcsp = @ph.longest_common_subpath(@dirs.keys)
@pid_hash = Concurrent::Hash.new

if (dtw = directories_to_watch).any?
if (@dtw = directories_to_watch).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 @@ -28,11 +29,11 @@ def initialize(files, dirs = {}, &block)
raise LoadError, "Could not load the 'listen' gem. Add `gem 'listen'` to the development group of your Gemfile", e.backtrace
end
end
Listen.to(*dtw, &method(:changed)).start
end
end

def updated?
boot! unless @pid_hash[Process.pid]
@updated.true?
end

Expand All @@ -50,6 +51,10 @@ def execute_if_updated
end

private
def boot!
Listen.to(*@dtw, &method(:changed)).start
@pid_hash[Process.pid] = true
end

def changed(modified, added, removed)
unless updated?
Expand Down
3 changes: 2 additions & 1 deletion activesupport/test/evented_file_update_checker_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ def setup
end

def new_checker(files = [], dirs = {}, &block)
ActiveSupport::EventedFileUpdateChecker.new(files, dirs, &block).tap do
ActiveSupport::EventedFileUpdateChecker.new(files, dirs, &block).tap do |c|
c.updated?
wait
end
end
Expand Down

0 comments on commit 7bd4199

Please sign in to comment.