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

Add new api optionally_get_with #187

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Conversation

LMJW
Copy link
Contributor

@LMJW LMJW commented Oct 23, 2022

This is the inital PR for issue #184

I was thinking to use TypeId with () type for the Error type when return InitResult, but not sure if this will have some conflict with other parts of the code as I saw () is use somewhere else. So I created internal structure OptionallyNone to represent this error type.

I also noticing the guarantee of call once only if the init closure return Ok/Some. If it returns Err/None, it is not gurantee to have once call. I think this definitely make sense, perhaps it is good to update the docummentation? Thanks

Let me know if there's any comments or suggestions. Thanks

@LMJW
Copy link
Contributor Author

LMJW commented Oct 23, 2022

@tatsuya6502 Feel free to take a look when you have time :)

@LMJW LMJW force-pushed the optionally-get-with branch from 1224598 to a256c42 Compare October 23, 2022 04:39
@LMJW
Copy link
Contributor Author

LMJW commented Oct 23, 2022

Hmm, looks like the failed CI is not related to the new API changes? https://github.com/moka-rs/moka/blob/master/src/common/concurrent/entry_info.rs#L145

@tatsuya6502
Copy link
Member

Thank you very much for the PR. I will review it when I have some spare time. (hopefully tonight, UTC+0800).

You are right. The failed CI is not related to this PR, so do not worry about it. It is already failing on the master branch when Rust 1.66 nightly is used. I am trying to fix it by #186.

@tatsuya6502 tatsuya6502 added this to the v0.9.5 milestone Oct 24, 2022
@tatsuya6502
Copy link
Member

I also noticing the guarantee of call once only if the init closure return Ok/Some. If it returns Err/None, it is not gurantee to have once call. I think this definitely make sense, perhaps it is good to update the docummentation? Thanks

Thank you for the suggestion. Please feel free to edit the documentation and submit a PR if you can make it better. That would be great help. I am not a native English speaker and having a bit struggle to write clear, concise and consistent English.

https://docs.rs/moka/latest/moka/sync/struct.Cache.html#method.try_get_with

This method prevents to evaluate the init closure multiple times on the same key even if the method is concurrently called by many threads; only one of the calls evaluates its closure ...

I think the word "even if" is a wrong choice here? It would imply "always". But what the cache actually does is coalescing concurrent queries.

  • Concurrent queries are coalesced into one evaluation of the init closure.
  • Sequential queries are not coalesced if they do not overlap.
  • The entry will not be in the cache (so the init closure should be evaluated) if:
    • The entry was never inserted:
      • including: The init closure of previous query returned Err/None.
    • The entry was inserted but then evicted by size a constraint or expiration (e.g. time-to-live).

Copy link
Member

@tatsuya6502 tatsuya6502 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 good to me! Thank you for adding the unit tests and documentation.

src/future/value_initializer.rs Show resolved Hide resolved
@tatsuya6502 tatsuya6502 merged commit 50d54f7 into moka-rs:master Oct 24, 2022
@tatsuya6502
Copy link
Member

@LMJW

I also noticing the guarantee of call once only if the init closure return Ok/Some. If it returns Err/None, it is not gurantee to have once call. I think this definitely make sense, perhaps it is good to update the docummentation?

FYI, I released v0.9.6 with updated document. I hope it will be more clear about the guarantee.

I also found that there are small chances for race conditions in concurrent calls of get_with, etc., which can make the init closure to be evaluated more than once. I believe I fixed it for v0.9.6 via #195.

The Documentation Update

Before

https://docs.rs/moka/0.9.5/moka/sync/struct.Cache.html#method.optionally_get_with

Try to ensure the value of the key exists by inserting an Some result of the init closure if not exist, and returns a clone of the value or None returned by the closure.

This method prevents to evaluate the init closure multiple times on the same key even if the method is concurrently called by many threads; only one of the calls evaluates its closure (as long as these closures return the same Option type), and other calls wait for that closure to complete.

After

https://docs.rs/moka/0.9.6/moka/sync/struct.Cache.html#method.optionally_get_with

Returns a clone of the value corresponding to the key. If the value does not exist, evaluates the init closure, and inserts the value if Some(value) was returned. If None was returned from the closure, this method does not insert a value and returns None.

Concurrent calls on the same key

This method guarantees that concurrent calls on the same not-existing key are coalesced into one evaluation of the init closure. Only one of the calls evaluates its closure, and other calls wait for that closure to complete.

The following code snippet demonstrates this behavior:

@LMJW
Copy link
Contributor Author

LMJW commented Nov 6, 2022

Hi @tatsuya6502 Thanks for letting me know. The change looks great!

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.

Add new api similar to try_get_with, but return Option rather than Result?
2 participants