From 4e9cb063a003ad54ce692f77d243e9c88238c035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix-Antoine=20Fortin?= Date: Tue, 12 Sep 2023 13:40:22 -0400 Subject: [PATCH 1/3] Make Digest subclass lookup thread-safe in lru_cache r10k content_synchronizer uses Forge v3 API when installing a module: https://github.com/puppetlabs/r10k/blob/4.0.0/lib/r10k/module/forge.rb#L95 https://github.com/puppetlabs/r10k/blob/4.0.0/lib/r10k/module/forge.rb#L128 https://github.com/puppetlabs/r10k/blob/4.0.0/lib/r10k/module/forge.rb#L176 r10k uses a thread pool to install the modules, therefore the Forge v3 API calls have to be thread safe, and so does the LruCache class used to cache API responses. If LruCache is left not thread safe, the same error as reported in r10k issue #979 happens. https://github.com/puppetlabs/r10k/issues/979 --- lib/puppet_forge/lru_cache.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet_forge/lru_cache.rb b/lib/puppet_forge/lru_cache.rb index da2c050..43df516 100644 --- a/lib/puppet_forge/lru_cache.rb +++ b/lib/puppet_forge/lru_cache.rb @@ -9,7 +9,7 @@ class LruCache # a convenience method for generating cache keys. Cache keys do not # have to be SHA256 hashes, but they must be unique. def self.new_key(*string_args) - Digest::SHA256.hexdigest(string_args.map(&:to_s).join(':')) + Digest(:SHA256).hexdigest(string_args.map(&:to_s).join(':')) end # @return [Integer] the maximum number of items to cache. From 833c162c3ddb1fc772c5d2fdb8fd353f27510448 Mon Sep 17 00:00:00 2001 From: Malik Parvez <84777619+malikparvez@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:31:24 +0530 Subject: [PATCH 2/3] Adding spec for thread safety --- spec/unit/forge/lru_cache_spec.rb | 66 ++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/spec/unit/forge/lru_cache_spec.rb b/spec/unit/forge/lru_cache_spec.rb index f33ddc5..380ea05 100644 --- a/spec/unit/forge/lru_cache_spec.rb +++ b/spec/unit/forge/lru_cache_spec.rb @@ -67,18 +67,26 @@ expect(cache.send(:lru)).to eq(['foo', 'baz']) end - # The below test is non-deterministic but I'm not sure how to unit - # test thread-safety. - # it 'is thread-safe' do - # cache = PuppetForge::LruCache.new - # cache.put('foo', 'bar') - # cache.put('baz', 'qux') - # threads = [] - # threads << Thread.new { 100.times { cache.get('foo') } } - # threads << Thread.new { 100.times { cache.get('baz') } } - # threads.each(&:join) - # expect(cache.send(:lru)).to eq(['baz', 'foo']) - # end + it 'is thread-safe for get calls' do + cache = PuppetForge::LruCache.new + + # Populate the cache with initial values + cache.put('foo', 'bar') + cache.put('baz', 'qux') + + # Create two threads for concurrent cache get operations + thread_one = Thread.new do + 100.times { expect(cache.get('foo')).to eq('bar') } + end + + thread_two = Thread.new do + 100.times { expect(cache.get('baz')).to eq('qux') } + end + + # Wait for both threads to complete + thread_one.join + thread_two.join + end end context '#put' do @@ -102,16 +110,30 @@ expect(cache.send(:lru)).to eq(['quux', 'baz']) end - # The below test is non-deterministic but I'm not sure how to unit - # test thread-safety. - # it 'is thread-safe' do - # cache = PuppetForge::LruCache.new - # threads = [] - # threads << Thread.new { 100.times { cache.put('foo', 'bar') } } - # threads << Thread.new { 100.times { cache.put('baz', 'qux') } } - # threads.each(&:join) - # expect(cache.send(:lru)).to eq(['baz', 'foo']) - # end + it 'is thread-safe' do + cache = PuppetForge::LruCache.new + + # Create two threads for concurrent cache operations + thread_one = Thread.new do + 100.times { cache.put('foo', 'bar') } + end + + thread_two = Thread.new do + 100.times { cache.put('baz', 'qux') } + end + + # Wait for both threads to complete + thread_one.join + thread_two.join + + # At this point, we don't need to compare the LRU list, + # because the order may change due to concurrent puts. + + # Instead, we simply expect the code to run without errors. + expect { thread_one.value }.not_to raise_error + expect { thread_two.value }.not_to raise_error + end + end context '#clear' do From 1f688497080d5dc173e814a776f4ccf2ac58236e Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Fri, 29 Sep 2023 10:19:53 -0700 Subject: [PATCH 3/3] Releasing v5.0.2 with thread safety fix --- CHANGELOG.md | 4 ++++ lib/puppet_forge/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d608a..a89a61b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ Starting with v2.0.0, all notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## v5.0.2 - 2023-09-29 +* Correct a Digest call making this thread-safe and allowing for concurrent r10k deploys. + Thanks to @cmd-ntrf for fixing it and to @baurmatt for tracking it down in the first place. + ## v5.0.1 - 2023-07-10 * Update README to reflect accurate Ruby requirement and `faraday` gem dependency ## v5.0.0 - 2023-05-07 diff --git a/lib/puppet_forge/version.rb b/lib/puppet_forge/version.rb index bca263e..64ae9bd 100644 --- a/lib/puppet_forge/version.rb +++ b/lib/puppet_forge/version.rb @@ -1,3 +1,3 @@ module PuppetForge - VERSION = '5.0.1' # Library version + VERSION = '5.0.2' # Library version end