Skip to content
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

Make Digest subclass lookup thread-safe in lru_cache #109

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

cmd-ntrf
Copy link
Contributor

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.
puppetlabs/r10k#979

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.
puppetlabs/r10k#979
@cmd-ntrf cmd-ntrf requested review from bastelfreak and a team as code owners September 12, 2023 17:55
@CLAassistant
Copy link

CLAassistant commented Sep 12, 2023

CLA assistant check
All committers have signed the CLA.

@bastelfreak
Copy link
Collaborator

@cmd-ntrf thanks for the PR. Any chance you can add a test for this?

@cmd-ntrf
Copy link
Contributor Author

@hsnodgrass
Copy link
Contributor

I'm not really sure how to unit test thread-safety in a deterministic manner. It looks like we could use https://github.com/yegor256/threads, but I have no experience with it.

@hsnodgrass
Copy link
Contributor

I did find this merged PR for Puma which makes the same change: puma/puma#2744. Personally, I think we're ok to merge this change without a specific test. This is a known issue in Ruby that I wasn't aware of when I wrote the cache

@alexjfisher
Copy link

Is this why I've just hit the following in r10k? (4.0.0 running in an AWS lambda function...)

[2023-09-20 15:54:34 - ERROR] Module selinux_core failed to synchronize due to Digest::Base cannot be directly inherited in Ruby
[2023-09-20 15:54:34 - ERROR] Error during concurrent deploy of a module: Digest::Base cannot be directly inherited in Ruby

@cmd-ntrf
Copy link
Contributor Author

cmd-ntrf commented Sep 20, 2023

Is this why I've just hit the following in r10k? (4.0.0 running in an AWS lambda function...)

yes. You can mitigate the problem by freezing puppet_forge gem version to 4.1.0 (i.e: gem install puppet_forge:4.1.0 r10k:4.0.0). The LruCache was introduced in 5.0.0.

Copy link
Member

@malikparvez malikparvez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@malikparvez malikparvez merged commit a6b95f4 into puppetlabs:main Sep 25, 2023
6 checks passed
@cmd-ntrf cmd-ntrf deleted the lru_digest_fix branch September 25, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants