From 7bd41994480c17db71fdc07e3447ade929eaa386 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 6 Jun 2016 11:26:22 -0500 Subject: [PATCH 1/5] EventedFileUpdateChecker boots once per process We need one file checker booted per process as talked about in #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 #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 #24990] [close #25259] --- .../active_support/evented_file_update_checker.rb | 15 ++++++++++----- .../test/evented_file_update_checker_test.rb | 3 ++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 21fdf7bb80..f549ce4040 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -13,11 +13,12 @@ module ActiveSupport @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. @@ -28,11 +29,11 @@ module ActiveSupport 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 @@ -50,6 +51,10 @@ module ActiveSupport end private + def boot! + Listen.to(*@dtw, &method(:changed)).start + @pid_hash[Process.pid] = true + end def changed(modified, added, removed) unless updated? diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index bc3f77bd54..ce2e05da2c 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -11,7 +11,8 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase 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 From bd38e92b41e055d107f4e4af8b60dfa6a38f444a Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 6 Jun 2016 11:26:50 -0500 Subject: [PATCH 2/5] [ci skip] document EventedFileUpdateChecker --- .../evented_file_update_checker.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index f549ce4040..c0ad21b0df 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -3,6 +3,34 @@ require 'pathname' require 'concurrent/atomic/atomic_boolean' module ActiveSupport + # Allows you to "listen" to changes in a file system. + # The evented file updater does not hit disk when checking for updates + # instead it uses platform specific file system events to trigger a change + # in state. + # + # The file checker takes an array of files to watch or a hash specifying directories + # and file extensions to watch. It also takes a block that is called when + # EventedFileUpdateChecker#execute is run or when EventedFileUpdateChecker#execute_if_updated + # is run and there have been changes to the file system. + # + # Note: To start listening to change events you must first call + # EventedFileUpdateChecker#updated? inside of each process. + # + # Example: + # + # checker = EventedFileUpdateChecker.new(["/tmp/foo"], -> { puts "changed" }) + # checker.updated? + # # => false + # checker.execute_if_updated + # # => nil + # + # FileUtils.touch("/tmp/foo") + # + # checker.updated? + # # => true + # checker.execute_if_updated + # # => "changed" + # class EventedFileUpdateChecker #:nodoc: all def initialize(files, dirs = {}, &block) @ph = PathHelper.new From a8f29f6455c303bbd321af248a730ca8b41f2f27 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 6 Jun 2016 15:24:56 -0500 Subject: [PATCH 3/5] Listen earlier in EventedFileUpdateChecker Some files like routes.rb may be very large and vary between the initialization of the app and the first request. In these scenarios if we are using a forked process we cannot rely on the files to be unchanged between when the code is booted and the listener is started. For that reason we start a listener on the main process immediately, when we detect that a process does not have a listener started we force the updated state to be true, so we are guaranteed to catch any changes made between the code initialization and the fork. --- .../active_support/evented_file_update_checker.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index c0ad21b0df..063839f5af 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -13,8 +13,7 @@ module ActiveSupport # EventedFileUpdateChecker#execute is run or when EventedFileUpdateChecker#execute_if_updated # is run and there have been changes to the file system. # - # Note: To start listening to change events you must first call - # EventedFileUpdateChecker#updated? inside of each process. + # Note: Forking will cause the first call to `updated?` to return `true`. # # Example: # @@ -41,10 +40,11 @@ module ActiveSupport @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) - @pid_hash = Concurrent::Hash.new + @block = block + @updated = Concurrent::AtomicBoolean.new(false) + @lcsp = @ph.longest_common_subpath(@dirs.keys) + @pid_hash = Concurrent::Hash.new + @parent_pid = Process.pid if (@dtw = directories_to_watch).any? # Loading listen triggers warnings. These are originated by a legit @@ -58,6 +58,7 @@ module ActiveSupport end end end + boot! end def updated? @@ -82,6 +83,7 @@ module ActiveSupport def boot! Listen.to(*@dtw, &method(:changed)).start @pid_hash[Process.pid] = true + @updated.make_true if @parent_pid != Process.pid end def changed(modified, added, removed) From 7d733b9f0f847d67d8759421f6820ebc57567647 Mon Sep 17 00:00:00 2001 From: schneems Date: Wed, 8 Jun 2016 15:52:02 -0500 Subject: [PATCH 4/5] Test how evented file checker handles forks Pretty proud of this. We are testing distributed processes synchronized via pipes which makes it deterministic. Pretty cool. We boot a listener in the parent process we then fork. Before we touch the file we verify the fork is booted using pipes. Then the parent process will touch the file while the fork waits on a pipe. Once the parent process signals that the file has been touched we continue inside of the fork. --- .../test/evented_file_update_checker_test.rb | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index ce2e05da2c..2cb2d8167f 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -12,7 +12,6 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase def new_checker(files = [], dirs = {}, &block) ActiveSupport::EventedFileUpdateChecker.new(files, dirs, &block).tap do |c| - c.updated? wait end end @@ -35,6 +34,48 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase super wait end + + test 'notifies forked processes' do + FileUtils.touch(tmpfiles) + + checker = new_checker(tmpfiles) { } + assert !checker.updated? + + # Pipes used for flow controll across fork. + boot_reader, boot_writer = IO.pipe + touch_reader, touch_writer = IO.pipe + + pid = fork do + assert checker.updated? + + # Clear previous check value. + checker.execute + assert !checker.updated? + + # Fork is booted, ready for file to be touched + # notify parent process. + boot_writer.write("booted") + + # Wait for parent process to signal that file + # has been touched. + IO.select([touch_reader]) + + assert checker.updated? + end + + assert pid + + # Wait for fork to be booted before touching files. + IO.select([boot_reader]) + touch(tmpfiles) + + # Notify fork that files have been touched. + touch_writer.write("touched") + + assert checker.updated? + + Process.wait(pid) + end end class EventedFileUpdateCheckerPathHelperTest < ActiveSupport::TestCase From 844af9fa7c18a0ee3316d6cf1289b144d48d84d7 Mon Sep 17 00:00:00 2001 From: schneems Date: Thu, 9 Jun 2016 16:42:20 -0500 Subject: [PATCH 5/5] Lock the whole boot step, get rid of unneeded hash --- .../active_support/evented_file_update_checker.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 063839f5af..a2dcf31132 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -43,8 +43,8 @@ module ActiveSupport @block = block @updated = Concurrent::AtomicBoolean.new(false) @lcsp = @ph.longest_common_subpath(@dirs.keys) - @pid_hash = Concurrent::Hash.new - @parent_pid = Process.pid + @pid = Process.pid + @boot_mutex = Mutex.new if (@dtw = directories_to_watch).any? # Loading listen triggers warnings. These are originated by a legit @@ -62,7 +62,13 @@ module ActiveSupport end def updated? - boot! unless @pid_hash[Process.pid] + @boot_mutex.synchronize do + if @pid != Process.pid + boot! + @pid = Process.pid + @updated.make_true + end + end @updated.true? end @@ -82,8 +88,6 @@ module ActiveSupport private def boot! Listen.to(*@dtw, &method(:changed)).start - @pid_hash[Process.pid] = true - @updated.make_true if @parent_pid != Process.pid end def changed(modified, added, removed)