From 8e1e670adb02aa21e14de504bcdf20e4b8bcf57c Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Fri, 28 Aug 2020 09:29:30 -0500 Subject: [PATCH] Make EventedFileUpdateChecker garbage collectable The Listen gem dispatches "file changed" events from a separate thread. This thread holds a reference to the `changed` callback, and runs until the listener is stopped. Thus, objects that are part of the `changed` callback's scope cannot be garbage collected until the listener is stopped. This commit isolates the `changed` callback and associated scope in an `EventedFileUpdateChecker::Core` instance. This ensures that the `EventedFileUpdateChecker` instance can be garbage collected. Additionally, this commit adds a finalizer to the `EventedFileUpdateChecker` instance, which stops the listener and ensures that the `EventedFileUpdateChecker::Core` instance can be garbage collected. --- .../evented_file_update_checker.rb | 98 +++++++++++-------- .../test/evented_file_update_checker_test.rb | 13 +++ 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 2a336a74eb..67138c116b 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -39,52 +39,31 @@ module ActiveSupport raise ArgumentError, "A block is required to initialize an EventedFileUpdateChecker" end - @ph = PathHelper.new - @files = files.map { |f| @ph.xpath(f) }.to_set - - @dirs = {} - dirs.each do |dir, exts| - @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 = Process.pid - @boot_mutex = Mutex.new - - dtw = directories_to_watch - @dtw, @missing = dtw.partition(&:exist?) - - boot! + @block = block + @pid = Process.pid + @core = Core.new(files, dirs) + ObjectSpace.define_finalizer(self, @core.finalizer) end def updated? - @boot_mutex.synchronize do + @core.mutex.synchronize do if @pid != Process.pid - boot! + @core.start @pid = Process.pid - @updated.make_true + @core.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 + if @core.restart? + @core.thread_safely(&:restart) + @core.updated.make_true end - @updated.true? + @core.updated.true? end def execute - @updated.make_false + @core.updated.make_false @block.call end @@ -96,18 +75,58 @@ module ActiveSupport end end - private - def boot! - normalize_dirs! + class Core + attr_reader :updated, :mutex + def initialize(files, dirs) + @ph = PathHelper.new + @files = files.map { |file| @ph.xpath(file) }.to_set + + @dirs = dirs.each_with_object({}) do |(dir, exts), hash| + hash[@ph.xpath(dir)] = Array(exts).map { |ext| @ph.normalize_extension(ext) }.to_set + end + + @common_path = @ph.longest_common_subpath(@dirs.keys) + + @dtw = directories_to_watch + @missing = [] + + @updated = Concurrent::AtomicBoolean.new(false) + @mutex = Mutex.new + + start + end + + def finalizer + proc { stop } + end + + def thread_safely + @mutex.synchronize do + yield self + end + end + + def start + normalize_dirs! + @dtw, @missing = [*@dtw, *@missing].partition(&:exist?) @listener = @dtw.any? ? Listen.to(*@dtw, &method(:changed)) : nil @listener&.start end - def shutdown! + def stop @listener&.stop end + def restart + stop + start + end + + def restart? + @missing.any?(&:exist?) + end + def normalize_dirs! @dirs.transform_keys! do |dir| dir.exist? ? dir.realpath : dir @@ -115,7 +134,7 @@ module ActiveSupport end def changed(modified, added, removed) - unless updated? + unless @updated.true? @updated.make_true if (modified + added + removed).any? { |f| watching?(f) } end end @@ -135,7 +154,7 @@ module ActiveSupport if matching && (matching.empty? || matching.include?(ext)) break true - elsif dir == @lcsp || dir.root? + elsif dir == @common_path || dir.root? break false end end @@ -154,6 +173,7 @@ module ActiveSupport @ph.filter_out_descendants(dtw) end + end class PathHelper def xpath(path) diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index 282f4ae65c..afdfa8c93b 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -2,6 +2,7 @@ require_relative "abstract_unit" require "pathname" +require "weakref" require_relative "file_update_checker_shared_tests" class EventedFileUpdateCheckerTest < ActiveSupport::TestCase @@ -77,6 +78,18 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase Process.wait(pid) end + test "can be garbage collected" do + previous_threads = Thread.list + checker_ref = WeakRef.new(ActiveSupport::EventedFileUpdateChecker.new([], tmpdir => ".rb") { }) + listener_threads = Thread.list - previous_threads + + wait # Wait for listener thread to start processing events. + GC.start + + assert_not_predicate checker_ref, :weakref_alive? + assert_empty Thread.list & listener_threads + end + test "should detect changes through symlink" do actual_dir = File.join(tmpdir, "actual") linked_dir = File.join(tmpdir, "linked")