-
-
Notifications
You must be signed in to change notification settings - Fork 635
fix: serialize generated pack regeneration #3231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ class PacksGenerator | |||||||
| # Auto-registration requires nested_entries support which was added in 7.0.0 | ||||||||
| # Note: The gemspec requires Shakapacker >= 6.0 for basic functionality | ||||||||
| MINIMUM_SHAKAPACKER_VERSION_FOR_AUTO_BUNDLING = "7.0.0" | ||||||||
| GENERATED_PACKS_LOCK_TTL_SECONDS = 120 | ||||||||
|
|
||||||||
| def self.instance | ||||||||
| @instance ||= PacksGenerator.new | ||||||||
|
|
@@ -48,26 +49,69 @@ def generate_packs_if_stale | |||||||
|
|
||||||||
| verbose = ENV["REACT_ON_RAILS_VERBOSE"] == "true" | ||||||||
|
|
||||||||
| add_generated_pack_to_server_bundle | ||||||||
| with_generated_packs_lock(verbose: verbose) do | ||||||||
| add_generated_pack_to_server_bundle | ||||||||
|
|
||||||||
| # Clean any non-generated files from directories | ||||||||
| clean_non_generated_files_with_feedback(verbose: verbose) | ||||||||
| # Clean any non-generated files from directories | ||||||||
| clean_non_generated_files_with_feedback(verbose: verbose) | ||||||||
|
|
||||||||
| are_generated_files_present_and_up_to_date = Dir.exist?(generated_packs_directory_path) && | ||||||||
| File.exist?(generated_server_bundle_file_path) && | ||||||||
| !stale_or_missing_packs? | ||||||||
| if generated_files_present_and_up_to_date? | ||||||||
| puts Rainbow("✅ Generated packs are up to date, no regeneration needed").green if verbose | ||||||||
| return | ||||||||
| end | ||||||||
|
|
||||||||
| if are_generated_files_present_and_up_to_date | ||||||||
| puts Rainbow("✅ Generated packs are up to date, no regeneration needed").green if verbose | ||||||||
| return | ||||||||
| clean_generated_directories_with_feedback(verbose: verbose) | ||||||||
| generate_packs(verbose: verbose) | ||||||||
| end | ||||||||
|
Comment on lines
+52
to
65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||
|
|
||||||||
| clean_generated_directories_with_feedback(verbose: verbose) | ||||||||
| generate_packs(verbose: verbose) | ||||||||
| end | ||||||||
|
|
||||||||
| private | ||||||||
|
|
||||||||
| def generated_files_present_and_up_to_date? | ||||||||
| Dir.exist?(generated_packs_directory_path) && | ||||||||
| File.exist?(generated_server_bundle_file_path) && | ||||||||
| !stale_or_missing_packs? | ||||||||
| end | ||||||||
|
|
||||||||
| def with_generated_packs_lock(verbose: false) | ||||||||
| lock_path = generated_packs_lock_path | ||||||||
| FileUtils.mkdir_p(lock_path.dirname) | ||||||||
| remove_stale_generated_packs_lock(lock_path, verbose: verbose) | ||||||||
|
|
||||||||
| File.open(lock_path, File::RDWR | File::CREAT, 0o644) do |lock_file| | ||||||||
| puts Rainbow("🔒 Waiting for generated packs lock at #{lock_path}").yellow if verbose | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "Waiting for generated packs lock" message is printed before
Suggested change
|
||||||||
| lock_file.flock(File::LOCK_EX) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider using a timeout approach, e.g. a background thread that sends a signal after N seconds, or polling with deadline = Time.now + GENERATED_PACKS_LOCK_TTL_SECONDS
loop do
break if lock_file.flock(File::LOCK_EX | File::LOCK_NB)
raise "Timed out waiting for generated packs lock" if Time.now > deadline
sleep 0.5
end |
||||||||
| lock_file.rewind | ||||||||
| lock_file.truncate(0) | ||||||||
| lock_file.write("pid=#{Process.pid}\nstarted_at=#{Time.now.utc}\n") | ||||||||
| lock_file.flush | ||||||||
|
|
||||||||
| yield | ||||||||
| ensure | ||||||||
| lock_file&.flock(File::LOCK_UN) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| def remove_stale_generated_packs_lock(lock_path, verbose: false) | ||||||||
| return unless File.exist?(lock_path) | ||||||||
| return unless File.mtime(lock_path) < Time.now - GENERATED_PACKS_LOCK_TTL_SECONDS | ||||||||
|
|
||||||||
| File.open(lock_path, File::RDWR) do |lock_file| | ||||||||
| next unless lock_file.flock(File::LOCK_EX | File::LOCK_NB) | ||||||||
|
|
||||||||
| FileUtils.rm_f(lock_path) | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Removing the lock path in Useful? React with 👍 / 👎. |
||||||||
| puts Rainbow("🧹 Removed stale generated packs lock at #{lock_path}").yellow if verbose | ||||||||
| ensure | ||||||||
| lock_file&.flock(File::LOCK_UN) | ||||||||
| end | ||||||||
|
Comment on lines
+99
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When |
||||||||
| rescue Errno::ENOENT | ||||||||
| nil | ||||||||
| end | ||||||||
|
|
||||||||
| def generated_packs_lock_path | ||||||||
| Rails.root.join("tmp", "react_on_rails_generate_packs.lock") | ||||||||
| end | ||||||||
|
|
||||||||
| def generate_packs(verbose: false) | ||||||||
| # Check for name conflicts between components and stores | ||||||||
| check_for_component_store_name_conflicts | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ def self.configuration | |
| after do | ||
| ReactOnRails.configuration.server_bundle_js_file = old_server_bundle | ||
| ReactOnRails.configuration.components_subdirectory = old_subdirectory | ||
| ReactOnRails.configuration.auto_load_bundle = old_auto_load_bundle | ||
|
|
||
| FileUtils.rm_rf "#{packer_source_entry_path}/generated" | ||
| FileUtils.rm_rf generated_server_bundle_file_path | ||
|
|
@@ -410,6 +411,44 @@ def self.rsc_support_enabled? | |
| expect(File.read(server_bundle_js_file_path).scan(/(?=#{test_string})/).count).to equal(1) | ||
| end | ||
|
|
||
| it "serializes concurrent generation and rechecks staleness after waiting" do | ||
| generator = described_class.new | ||
| generation_started = Queue.new | ||
| release_generation = Queue.new | ||
| second_completed = Queue.new | ||
| generation_count = 0 | ||
|
|
||
| allow(generator).to receive(:add_generated_pack_to_server_bundle) | ||
| allow(generator).to receive(:clean_non_generated_files_with_feedback) | ||
| allow(generator).to receive(:clean_generated_directories_with_feedback) | ||
| allow(generator).to receive(:generated_files_present_and_up_to_date?).and_return(false, true) | ||
| allow(generator).to receive(:generate_packs) do | ||
| generation_count += 1 | ||
| generation_started << true | ||
| release_generation.pop | ||
| end | ||
|
|
||
| first_thread = Thread.new { generator.generate_packs_if_stale } | ||
| generation_started.pop | ||
|
|
||
| second_thread = Thread.new do | ||
| generator.generate_packs_if_stale | ||
| second_completed << true | ||
| end | ||
|
|
||
| sleep 0.1 | ||
| expect { second_completed.pop(true) }.to raise_error(ThreadError) | ||
|
Comment on lines
+439
to
+440
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This A more robust approach is to use a |
||
|
|
||
| release_generation << true | ||
| first_thread.join | ||
| second_thread.join | ||
|
Comment on lines
+439
to
+444
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| expect(generation_count).to eq(1) | ||
| ensure | ||
| first_thread&.kill if first_thread&.alive? | ||
| second_thread&.kill if second_thread&.alive? | ||
| end | ||
|
|
||
| it "generate packs if a new component is added" do | ||
| create_new_component("NewComponent") | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
returninside ayield-based block works correctly here — Ruby propagates it as a method-return fromgenerate_packs_if_stale, and theensurein theFile.openblock still runs, properly releasing the lock. However, this is a subtle Ruby behavior that could surprise contributors who refactor this code (e.g., wrapping the block in aProcinstead ofyieldwould turn it into aLocalJumpError).A less surprising alternative is to use a boolean guard:
This avoids relying on
return-from-block semantics entirely.