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 concurrent memoization of zero-arg methods multiple values #230

Merged
merged 1 commit into from Dec 3, 2021

Conversation

ms-ati
Copy link
Contributor

@ms-ati ms-ati commented Nov 18, 2021

When two threads concurrently call a zero-arg method which is in
the unmemoized state, they can return two different results.

For example, this could occur if we are memoizing:

  • database query result
  • filesystem read
  • current time
  • a random number

This PR adds documentation that this is expected behavior to README.md,
as well as a probabilistic spec that demonstrates the behavior.

@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #230 (6bdfea1) into main (f2cdecf) will not change coverage.
The diff coverage is n/a.

❗ Current head 6bdfea1 differs from pull request most recent head 785856a. Consider uploading reports for the commit 785856a to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #230   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          201       201           
=========================================
  Hits           201       201           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2cdecf...785856a. Read the comment docs.

@ms-ati
Copy link
Contributor Author

ms-ati commented Nov 19, 2021

As expected: this purely ruby code "double check" pattern, where we check again whether the memoization sentinel has been set in a concurrent thread after we have run the original method, appears to be successful in all test runs on all MRI versions, but sometimes fails on JRuby.

Options

  1. We could leave the code in its prior state, and document that contended calls can result in different return value across threads on all Ruby versions
  2. We could adopt this solution for MRI, and document that it doesn't work on JRuby in both markdown and tests (e.g. by marking this test as "pending" in JRuby)
  3. We might go a step further and use a Mutex on each object, which would fix this issue on all Ruby versions, at some additional cost on those first calls where we set the memoized value

I'm hoping that this PR can help kick off a discussion and we can intentionally select an option 😄

@ms-ati
Copy link
Contributor Author

ms-ati commented Nov 30, 2021

Summary of discussion with @jemmaissroff and @JacobEvelyn outside of Github:

  • We've selected Option 1: leave code in its prior state, and document that contended calls can result in different return values across threads
  • Why?
      1. Because memoization of a non-deterministic method is an app-by-app decision. It's not a memoization library's place to institute a single policy and expectation of what should be memoized and returned in each thread, whether any given concurrency primitive should be employed, etc
      1. Because memoization of a deterministic method already works safely under contention, although the underlying method may be called multiple times
  • Next steps:
    • Document the behavior via both writing and specs
    • Add examples of how the code that uses MemoWise can employ a Mutex if desired

@ms-ati ms-ati force-pushed the fix-thread-safety-zero-arg-multiple-values branch 4 times, most recently from 9174449 to 1cccc6d Compare December 3, 2021 17:07
Copy link
Member

@JacobEvelyn JacobEvelyn left a comment

Choose a reason for hiding this comment

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

Just some minor comments. Thanks for working on this!

Also, consider adding this to the bottom of your commit message:

Closes #184 

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spec/thread_safety_spec.rb Outdated Show resolved Hide resolved
spec/thread_safety_spec.rb Outdated Show resolved Hide resolved
spec/thread_safety_spec.rb Outdated Show resolved Hide resolved
spec/thread_safety_spec.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ms-ati ms-ati force-pushed the fix-thread-safety-zero-arg-multiple-values branch 2 times, most recently from 6bdfea1 to 9a1f471 Compare December 3, 2021 19:47
Document, via both README and specs, that when two threads concurrently
call a zero-arg method which is in the unmemoized state, they can return
two different results.

For example, this could occur if we are memoizing:
  * database query result
  * filesystem read
  * current time
  * a random number

This PR introduces a test which probabilistically reproduces the
issue, as well as documentation that this is the expected behavior.

Disables a test on TruffleRuby in CI, as it takes multiple seconds
to observe the different return values there.

Closes #184
@ms-ati ms-ati force-pushed the fix-thread-safety-zero-arg-multiple-values branch from 9a1f471 to 785856a Compare December 3, 2021 20:03
@ms-ati ms-ati merged commit a97cd40 into main Dec 3, 2021
@ms-ati ms-ati deleted the fix-thread-safety-zero-arg-multiple-values branch December 3, 2021 20:11
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.

None yet

2 participants