diff --git a/docs/src/guide/development.md b/docs/src/guide/development.md index bea1de3b..e860ee2f 100644 --- a/docs/src/guide/development.md +++ b/docs/src/guide/development.md @@ -112,6 +112,10 @@ automatically when the asset is requested. This is very convenient when running integration tests, or when a developer does not want to start the Vite development server (at the expense of a slower feedback loop). +When tests run in parallel processes, builds and manifest reads are synchronized +across workers. The first worker builds stale assets while the others wait and +reuse its result. + ::: tip Enabled locally By [default][json config], [autoBuild] is enabled in the `test` and `development` environments. ::: @@ -178,10 +182,14 @@ When running tests locally, you can test the production build by not starting th When running tests in the CI, it's more reliable if assets are available __before__ tests start to run, as it: -- Prevents timeouts in Capybara during [autoBuild] -- Prevents race conditions when running tests in parallel (each thread could start a build) +- Prevents timeouts in Capybara while waiting for [autoBuild] +- Verifies the production asset build independently from the test suite + +Parallel test workers are synchronized when using [autoBuild], so +precompilation is not required for correctness. It remains recommended in CI +for predictable test startup and build failures. -To achieve that, it's recommended to run `bin/rake assets:precompile`—which should [also run `vite build`][deployment]—in a previous CI step. +To achieve that, run `bin/rake assets:precompile`—which should [also run `vite build`][deployment]—in a previous CI step. You can verify your setup is working by disabling [autoBuild]. A convenient way to do that is to add `VITE_RUBY_AUTO_BUILD="false"` to the build environment variables. diff --git a/test/builder_test.rb b/test/builder_test.rb index b189502b..5dd10cdb 100644 --- a/test/builder_test.rb +++ b/test/builder_test.rb @@ -81,6 +81,12 @@ def test_last_build_path assert_equal builder.send(:last_build_path, ssr: true).basename.to_s, "last-ssr-build-#{ViteRuby.config.mode}.json" end + def test_build_is_synchronized_between_processes + skip "Process.fork is not supported" unless Process.respond_to?(:fork) + + assert_build_is_synchronized_between_processes + end + def test_watched_files_digest previous_digest = ViteRuby.digest refresh_config @@ -158,6 +164,39 @@ def test_build_cache private + def assert_build_is_synchronized_between_processes + build_count_path = ViteRuby.config.build_cache_dir.join("build-count") + build_count_path.dirname.mkpath + reader, writer = IO.pipe + result = ["stdout", "", MockProcessStatus.new(success: true)] + + ViteRuby::IO.stub(:capture, ->(*) { + build_count_path.open("a") { |file| file.puts Process.pid } + sleep 0.2 + result + }) do + pids = Array.new(2) do + Process.fork do + writer.close + reader.read(1) + exit! builder.build ? 0 : 1 + end + end + + reader.close + 2.times { writer.write("1") } + writer.close + + assert pids.map { |pid| Process.wait2(pid).last }.all?(&:success?) + end + + assert_equal 1, build_count_path.readlines.size + ensure + reader&.close unless reader&.closed? + writer&.close unless writer&.closed? + build_count_path&.delete if build_count_path&.exist? + end + def watched_file Pathname.new(path_to_test_app).join(WATCHED_FILE) end diff --git a/test/manifest_test.rb b/test/manifest_test.rb index 516196b6..050cab62 100644 --- a/test/manifest_test.rb +++ b/test/manifest_test.rb @@ -117,6 +117,16 @@ def test_lookup_with_type_exception! assert_match "Vite Ruby can't find entrypoints/#{asset_file}.js in the manifests", error.message end + def test_lookup_does_not_initialize_build_dependencies_when_auto_build_is_disabled + refute ViteRuby.instance.instance_variable_defined?(:@builder) + refute ViteRuby.instance.instance_variable_defined?(:@build_lock) + + assert_equal prefixed("app.517bf154.css"), path_for("app", type: :stylesheet) + + refute ViteRuby.instance.instance_variable_defined?(:@builder) + refute ViteRuby.instance.instance_variable_defined?(:@build_lock) + end + def test_lookup_success! vendor_chunk = { "file" => prefixed("vendor.0f7c0ec3.js"), @@ -263,6 +273,12 @@ def test_skip_proxy_has_no_effect_without_dev_server assert_equal prefixed("logo.f42fb7ea.png"), path_for("images/logo.png") end + def test_manifest_read_waits_for_build_in_another_process + skip "Process.fork is not supported" unless Process.respond_to?(:fork) + + assert_manifest_read_waits_for_build_in_another_process + end + def test_lookup_nil assert_nil lookup("foo.js") end @@ -302,6 +318,40 @@ def test_lookup_success private + def assert_manifest_read_waits_for_build_in_another_process + refresh_config(auto_build: true) + path = ViteRuby.config.manifest_paths.first + contents = path.read + reader, writer = IO.pipe + + pid = Process.fork do + reader.close + ViteRuby.instance.build_lock.synchronize(File::LOCK_EX) do + path.write("") + writer.write("1") + writer.close + sleep 0.2 + path.write(contents) + end + exit! 0 + end + + writer.close + reader.read(1) + + assert ViteRuby.instance.manifest.refresh + assert_predicate Process.wait2(pid).last, :success? + ensure + reader&.close unless reader&.closed? + writer&.close unless writer&.closed? + path&.write(contents) if path && contents + begin + Process.wait(pid) if pid + rescue Errno::ECHILD + nil + end + end + def assert_raises_manifest_missing_entry_error(auto_build: false, &block) error = nil diff --git a/vite_ruby/lib/vite_ruby.rb b/vite_ruby/lib/vite_ruby.rb index 71a59ae8..02f72136 100644 --- a/vite_ruby/lib/vite_ruby.rb +++ b/vite_ruby/lib/vite_ruby.rb @@ -113,6 +113,11 @@ def run(argv, **options) (@runner ||= ViteRuby::Runner.new(self)).run(argv, **options) end + # Internal: Synchronizes Vite builds and manifest reads. + def build_lock + @build_lock ||= ViteRuby::BuildLock.new(self) + end + # Public: Keeps track of watched files and triggers builds as needed. def builder @builder ||= ViteRuby::Builder.new(self) diff --git a/vite_ruby/lib/vite_ruby/build_lock.rb b/vite_ruby/lib/vite_ruby/build_lock.rb new file mode 100644 index 00000000..ef96a4f9 --- /dev/null +++ b/vite_ruby/lib/vite_ruby/build_lock.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +# Internal: Synchronizes Vite builds and manifest reads across threads and processes. +class ViteRuby::BuildLock + def initialize(vite_ruby) + @vite_ruby = vite_ruby + @mutex = Mutex.new + end + + def synchronize(mode) + @mutex.synchronize do + lock_path.dirname.mkpath + lock_path.open(File::RDWR | File::CREAT, 0o644) do |lock| + lock.flock(mode) + yield + end + end + end + +private + + extend Forwardable + + def_delegator :@vite_ruby, :config + + # The lock is kept outside the build cache so clobbering the cache cannot + # replace the locked file while another process is waiting on it. + def lock_path + Pathname.new("#{config.build_cache_dir}.lock") + end +end diff --git a/vite_ruby/lib/vite_ruby/builder.rb b/vite_ruby/lib/vite_ruby/builder.rb index bfdefe71..1a0483c5 100644 --- a/vite_ruby/lib/vite_ruby/builder.rb +++ b/vite_ruby/lib/vite_ruby/builder.rb @@ -12,19 +12,21 @@ def initialize(vite_ruby) # Public: Checks if the watched files have changed since the last compilation, # and triggers a Vite build if any files have changed. def build(*args) - last_build = last_build_metadata(ssr: args.include?("--ssr")) - - if args.delete("--force") || last_build.stale? || config.manifest_paths.empty? - stdout, stderr, status = build_with_vite(*args) - log_build_result(stdout, stderr, status) - record_build_metadata(last_build, errors: stderr, success: status.success?) - status.success? - elsif last_build.success - logger.debug "Skipping vite build. Watched files have not changed since the last build at #{last_build.timestamp}" - true - else - logger.error "Skipping vite build. Watched files have not changed since the build failed at #{last_build.timestamp} ❌" - false + build_lock.synchronize(File::LOCK_EX) do + last_build = last_build_metadata(ssr: args.include?("--ssr")) + + if args.delete("--force") || last_build.stale? || config.manifest_paths.empty? + stdout, stderr, status = build_with_vite(*args) + log_build_result(stdout, stderr, status) + record_build_metadata(last_build, errors: stderr, success: status.success?) + status.success? + elsif last_build.success + logger.debug "Skipping vite build. Watched files have not changed since the last build at #{last_build.timestamp}" + true + else + logger.error "Skipping vite build. Watched files have not changed since the build failed at #{last_build.timestamp} ❌" + false + end end end @@ -37,7 +39,7 @@ def last_build_metadata(ssr: false) extend Forwardable - def_delegators :@vite_ruby, :config, :logger, :run + def_delegators :@vite_ruby, :build_lock, :config, :logger, :run # Internal: Writes a digest of the watched files to disk for future checks. def record_build_metadata(build, **attrs) diff --git a/vite_ruby/lib/vite_ruby/manifest.rb b/vite_ruby/lib/vite_ruby/manifest.rb index 1a151323..b22324a6 100644 --- a/vite_ruby/lib/vite_ruby/manifest.rb +++ b/vite_ruby/lib/vite_ruby/manifest.rb @@ -12,7 +12,6 @@ class ViteRuby::Manifest def initialize(vite_ruby) @vite_ruby = vite_ruby - @build_mutex = Mutex.new if config.auto_build end # Public: Returns the path for the specified Vite entrypoint file. @@ -104,7 +103,9 @@ def lookup!(name, **options) # manifest.lookup('calendar.js') # => { "file" => "/vite/assets/calendar-1016838bab065ae1e122.js", "imports" => [] } def lookup(name, **options) - @build_mutex.synchronize { builder.build || (return nil) } if should_build? + if should_build? + return unless builder.build + end find_manifest_entry resolve_entry_name(name, **options) end @@ -116,7 +117,7 @@ def lookup(name, **options) extend Forwardable - def_delegators :@vite_ruby, :config, :builder, :dev_server_running? + def_delegators :@vite_ruby, :build_lock, :config, :builder, :dev_server_running? # NOTE: Auto compilation is convenient when running tests, when the developer # won't focus on the frontend, or when running the Vite server is not desired. @@ -145,6 +146,12 @@ def manifest # Internal: Loads and merges the manifest files, resolving the asset paths. def load_manifest + return read_manifest unless config.auto_build + + build_lock.synchronize(File::LOCK_SH) { read_manifest } + end + + def read_manifest config.manifest_paths .map { |path| JSON.parse(path.read) } .inject({}, &:merge)