Update dependency moka to v0.12.1 #7075
Conversation
|
☔ The latest upstream changes (presumably 3d61a79) made this pull request unmergeable. Please resolve the merge conflicts. |
tatsuya6502
force-pushed
the
moka-v0.12
branch
from
2860b0e
to
2b65ec2
Compare
|
☔ The latest upstream changes (presumably #7123) made this pull request unmergeable. Please resolve the merge conflicts. |
tatsuya6502
force-pushed
the
moka-v0.12
branch
2 times, most recently
from
571dafb
to
f060283
Compare
|
I released moka v0.12.0 two days ago, and the number of v0.12.0 downloads is growing, accounting for about 11.6% of the total downloads yesterday. I will continue monitoring the download numbers.
There were significant internal changes in v0.12.0. I did some additional testing and code review last week, but I am still a bit concerned about regressions (e.g. causing fatal failures like deadlocks). As for crates.io, I do not think there is any rush to upgrade moka, so we will probably wait two or more weeks before upgrading? Let's wait and see if other v0.12.0 users report any problems. I will give you an update in early October, or when any problems are reported. |
sounds good to me, thanks! :) |
|
Unfortunately, an issue was reported for moka v0.12. We are looking into it now. |
|
☔ The latest upstream changes (presumably f6d24d8) made this pull request unmergeable. Please resolve the merge conflicts. |
tatsuya6502
force-pushed
the
moka-v0.12
branch
from
f060283
to
a654eb5
Compare
tatsuya6502
changed the title
We identified a bug introduced in moka v0.12.0 and fixed it. We have published moka v0.12.1 with the fix and I updated this pull request to use it. Let's wait for few more days to see if moka v0.12.1 fixes the memory leak at the user side. https://github.com/moka-rs/moka/blob/v0.12.1/CHANGELOG.md#version-0121
|
It looks good; they are seeing no sign of memory leak in moka v0.12.1. Just for sure, I will wait until Tuesday afternoon in my time zone (UTC+0800), and if no other issues are reported, I will mark this pull request ready for review. |
|
☔ The latest upstream changes (presumably 0d8472a) made this pull request unmergeable. Please resolve the merge conflicts. |
tatsuya6502
force-pushed
the
moka-v0.12
branch
from
a654eb5
to
98bd86c
Compare
|
👋 since I will be at the EuroRust conference starting tomorrow, I most likely won't have time to monitor the production environment sufficiently enough to merge this just yet. we might have to delay this until next week it that's okay with you :) |
Yes, it is okay. Let's delay it until next week or whenever convenient for you. I will watch EuroRust online. Arpad Borsos (Swatinem) will mention moka in his session as he found the |
|
☔ The latest upstream changes (presumably 88ebde4) made this pull request unmergeable. Please resolve the merge conflicts. |
Turbo87
changed the title
|
I've rebased this to fix the merge conflict with the lockfile update. I'm currently deploying this to our staging environment to test it out there and will hopefully merge this afterwards. |
|
I've tested the PR on the staging environment and couldn't see any immediate issues. I'll merge this now and will deploy it to production afterwards. |
| // by "Cannot start a runtime from within a runtime"). Here, we are in | ||
| // a spawn_blocking call because of conduit_compat, so our current thread | ||
| // is not an executor of the runtime. | ||
| rt.block_on(app.version_id_cacher.insert(cache_key, version_id)) |
There was a problem hiding this comment.
@tatsuya6502 is there a reason why you extracted the rt variable outside of the conduit_compat() closure instead of using Handle::current().block_on(...) directly here? we've been using Handle::current().block_on(...) in a few other places and I'm trying to figure out if that is a mistake or not 😅
There was a problem hiding this comment.
Hi. I did not realize that you already had Handle::current().block_on(...) in other places. I never tried doing so, and I am not sure if it is okay or not.
I had an impression that Handle::current() should be called in async context. But I just checked Tokio's document and it seems I was wrong. It says the followings:
https://docs.rs/tokio/1.33.0/tokio/runtime/struct.Handle.html#method.current
pub fn current() -> SelfReturns a
Handleview over the currently running Runtime.Panics
This will panic if called outside the context of a Tokio runtime. That means that you must call this on one of the threads being run by the runtime, or from a thread with an active
EnterGuard.
It says "outside the context of Tokio runtime", not "async context". conduit_compat runs the given closure in tokio::task::spawn_blocking, which uses a pool of threads being run by Tokio runtime. So, if understand correctly, calling Handle::current() should be okay there.
There was a problem hiding this comment.
okay, thanks for the clarification. I guess we'll try it out and see what happens. The test suite seems to work fine that way at least.
Please do not merge this pull request yet.
Hi. We are planning to release a new version of moka cache sometime soon, hopefully in a couple of weeks from now. It has major breaking changes (for good reasons) and this PR updates the codes for it. v0.12.0 is currently in beta so this pull request (PR) is depending on the latest v0.12.0-beta.2 for now.
For details of the breaking changes, please see the MIGRATION-GUIDE.md.
I will let you know when moka v0.12.0 is released, but since this is the biggest change in moka's history (e.g. one of the PRs had +5,624 −963 lines in the diff), I am a bit concerned if we have introduced any regressions. Perhaps, you may want to hold off merging this PR until we become confident about v0.12.0.
Thanks!