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 sure that concurrent map usage is thread-safe #46536

Merged
merged 1 commit into from
Nov 20, 2022

Conversation

mensfeld
Copy link
Contributor

Behavior upon missing prefix partial name may cause a key to overwrite when executed in multiple threads at the same time.

ref ruby-concurrency/concurrent-ruby#970

Motivation / Background

Same as here: #46534

But since guidelines state to isolate PRs, hence the second one.

Detail

This Pull Request changes the way a cache miss is handled. It makes it thread safe and ensures that it's not ovewritten in the middle of execution.

Additional information

ref: #46534
ref: ruby-concurrency/concurrent-ruby#970

Checklist

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
  • CI is passing.

Behavior upon missing prefix partial name may cause a key to overwrite when executed in multiple threads at the same time.

ref ruby-concurrency/concurrent-ruby#970
@rails-bot rails-bot bot added the actionview label Nov 20, 2022
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

It looks like PREFIXED_PARTIAL_NAMES has been this way for quite a while (since #8510). Because it is only a cache, any values lost to race conditions are simply recomputed (and kept thereafter), so it has not caused any problems. Still, fixing it seems like a good idea. 👍

@jonathanhefner jonathanhefner merged commit eb303ad into rails:main Nov 20, 2022
@jonathanhefner
Copy link
Member

Thank you, @mensfeld! 🎉

@mensfeld mensfeld deleted the patch-2 branch November 20, 2022 22:07
@mensfeld
Copy link
Contributor Author

Wow. Does it mean I became a rails contributor? 😅 I didn't expect that to happen!

Because it is only a cache, any values lost to race conditions are simply recomputed (and kept thereafter), so it has not caused any problems

I am aware of this, but my second reason for fixing this was to promote a proper design. We all learn from each other's code and having this that way can make other people think it is the correct way to operate those data structures. This is how I found this in the first place myself: used this "typical" design, stumbled upon complex, hard to debug error and spent a lot of time looking at other cases. Anyhow, glad I could help 💪

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.

None yet

2 participants