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

Fix memoization thread safety #30

Merged
merged 6 commits into from
Apr 4, 2024
Merged

Fix memoization thread safety #30

merged 6 commits into from
Apr 4, 2024

Conversation

mkdynamic
Copy link
Contributor

I haven’t tested this (edited this PR on my phone) but in theory this should fix the thread safety issue.

@mkdynamic mkdynamic mentioned this pull request Mar 23, 2024
@ScotterC
Copy link
Collaborator

Thanks for the contribution!
I haven't had much experience with writing a spec for thread safety issues. Have a sense of the outline you'd approach it with?

@mkdynamic
Copy link
Contributor Author

Unit testing reliably this would very tricky. IMO it’s not worth it, and you’d mainly end up testing the Ruby Mutex works, which seems pointless.

This pattern of using a mutex to synchronize a memoized class variable in Ruby is common enough — I would say if the existing test suite passes, call it good.

@IAPark
Copy link
Owner

IAPark commented Mar 25, 2024

Could you run rake standard:fix?

I'm guessing we could do this with less locking, maybe eager looking in the cache before locking, but probably safest to just use a mutex for the whole thing

As to testing I agree that there aren't really great options. I think I've mostly heard of people using locks under the hood to make sure the race condition happens, but I think I'm comfortable if all the specs pass with this.

@andreibondarev
Copy link

Relevant issue and comment: patterns-ai-core/langchainrb#446 (comment)

@IAPark IAPark merged commit f2930bf into IAPark:main Apr 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants