-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
bug(lib): drop env on last use #2011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok with me, can you just remove this dbg
, please?
meilisearch-lib/src/snapshot.rs
Outdated
@@ -90,7 +90,7 @@ pub struct SnapshotJob { | |||
|
|||
impl SnapshotJob { | |||
pub async fn run(self) -> anyhow::Result<()> { | |||
tokio::task::spawn_blocking(|| self.run_sync()).await??; | |||
tokio::task::spawn_blocking(|| dbg!(self.run_sync())).await??; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tokio::task::spawn_blocking(|| dbg!(self.run_sync())).await??; | |
tokio::task::spawn_blocking(|| self.run_sync()).await??; |
keys: Database<ByteSlice, SerdeJson<Key>>, | ||
action_keyid_index_expiration: Database<KeyIdActionCodec, SerdeJson<Option<DateTime<Utc>>>>, | ||
} | ||
|
||
impl Drop for HeedAuthStore { | ||
fn drop(&mut self) { | ||
if Arc::strong_count(&self.env) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider it safe to check that the count is equal to 1 as there is no way it can grow between the moment you acquire this number and you compare it to 1.
b12769b
to
ad2a588
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the dbg!
in the snapshot.rs
file, please?
My bad I pushed an empty commit --' |
fixes the `too many open files` error when running tests by closing the environment on last drop
ad2a588
to
b28a465
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
bors merge |
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
bors merge |
2011: bug(lib): drop env on last use r=curquiza a=MarinPostma fixes the `too many open files` error when running tests by closing the environment on last drop To check that we are actually the last owner of the `env` we plan to drop, I have wrapped all envs in `Arc`, and check that we have the last reference to it. Co-authored-by: Marin Postma <postma.marin@protonmail.com>
Build failed: |
bors merge |
2011: bug(lib): drop env on last use r=curquiza a=MarinPostma fixes the `too many open files` error when running tests by closing the environment on last drop To check that we are actually the last owner of the `env` we plan to drop, I have wrapped all envs in `Arc`, and check that we have the last reference to it. Co-authored-by: Marin Postma <postma.marin@protonmail.com>
Build failed: |
bors merge |
fixes the
too many open files
error when running tests by closing theenvironment on last drop
To check that we are actually the last owner of the
env
we plan to drop, I have wrapped all envs inArc
, and check that we have the last reference to it.