search

rpm-software-management / librpm.rs

Rust bindings to the RPM Package Manager libraries (librpm, librpmbuild, librpmsign)
https://rustrpm.org/librpm/
Mozilla Public License 2.0
31 stars 14 forks source link

When running twice, messages about stale read locks appear #12

Open mkpankov opened 4 years ago

mkpankov commented 4 years ago 
[root@3f9fc1f2b0bb work]# cargo run --target-dir docker-target
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
     Running `docker-target/debug/rpmp`
true
[root@3f9fc1f2b0bb work]# cargo run --target-dir docker-target
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `docker-target/debug/rpmp`
BDB2053 Freeing read locks for locker 0xff: 15/140056836946176
BDB2053 Freeing read locks for locker 0x101: 15/140056836946176
true

This indicates improper clean up of resources I believe. Happens on current master.

I've set up a repository to easily reproduce this bug.

tarcieri commented 4 years ago

That's unfortunate. This library has been written to use a relatively simple and conservative API strategy based on discussions with RPM developers about how to, given their massive amount of hindsight regarding bad librpm bindings from other languages, pursue as simple as possible a solution, and pretty much everything I implemented is from their suggestions.

My best guess is this is tied to the way this library acquires but never releases the global transaction set. It originally did, with an owned TransactionSet as part of the public API. It was refactored into a global static singleton in this PR (when unfortunately the code lived in a different repo):

https://github.com/iqlusioninc/crates/pull/28/files

That was at the suggestion of one of the RPM5 developers, however unfortunately it seems he deleted all of his comments so you're unfortunately getting only my side of the conversation and all of the tidbits related in the thread seem to be lost 😢

https://github.com/rpm-software-management/rpm/issues/429#issuecomment-381417575

That particular change ended up being very nice for the API ergonomics. It eliminated all of the lifetimes around transaction sets by making it 'static, and thread safety concerns by wrapping access in a Mutex.

I can try poking at your repro to confirm this, but if that's the case, it seems like either it needs to be reverted to the owned value-based TransactionSet API, or some sort of "at exit" handler needs to be added to end the global transaction set?

Otherwise it might be the iterator leaking...

mkpankov commented 4 years ago

Thank you for extensive answer.

I consider not leaking resources above any ergonomics to be honest.

Isn't the "at exit handler" just Drop implementation?

tarcieri commented 4 years ago

TransactionSet has a (legacy) Drop impl:

https://github.com/RustRPM/librpm.rs/blob/13ce47/src/internal/ts.rs

However it's stored in a lazy_static:

https://github.com/RustRPM/librpm.rs/blob/13ce47/src/internal/global_state.rs#L14

and since it has a 'static lifetime, it's never dropped.

One option might be to use something like the ctor crate, defining a dtor that invokes librpm_sys::rpmtsFree at exit (perhaps removing the current Drop impl on TransactionSet).