Possible memory leak in crossbeam
rolag opened this issue · 9 comments
Unless I'm misunderstanding something in this crate, when I compile and run this simple gist in valgrind, I get the following "possibly lost" and "still reachable" leaks:
==14743== LEAK SUMMARY:
==14743== definitely lost: 0 bytes in 0 blocks
==14743== indirectly lost: 0 bytes in 0 blocks
==14743== possibly lost: 25,440 bytes in 82 blocks
==14743== still reachable: 848,610 bytes in 1,323 blocks
==14743== suppressed: 0 bytes in 0 blocks
==14743==
==14743== For counts of detected and suppressed errors, rerun with: -v
==14743== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)
These numbers only seem to increase as the number of loop iterations increases.
The full valgrind log is in the gist.
Running rustc 1.26.0-dev (nightly) with the system allocator on Linux.
Threads spawned in main are never joined, which means they'll be forcibly killed at the end of the program. This is a big source of leaks. I suggest doing the following instead:
fn main() {
crossbeam::scope(|scope| {
for _ in 0..100 {
scope.spawn(test);
}
});
}Another problem is that the default collector is never dropped since it is a global variable. If you create a fresh collector in test (with let c = epoch::Collector::new() and replace epoch::pin() with c.handle().pin(), there will be no leaks.
I wonder if we could/should somehow clean up memory from the default collector when all threads exit. Maybe we should clear the garbage queue when all handles to the global collector are gone?
cc @jeehoonkang
Alternatively you could just join() the threads in a second loop after having spawned them. This should eliminate all the "possibly lost" memory. Having some memory as "still reachable" when the program exits is fine.
@stjepang I don't feel too strongly either way about cleaning up the garbage queue at exit. On one hand it would be "cleaner" to do so, but then again nobody cares if you don't free memory when the process is about to be killed anyways. In any case this only affects the global Collector which is initialized with lazy_static!, custom collectors should be properly cleaned up once their reference count reaches zero.
You're right, using scopes and a local collector seems to give no more "possibly lost" leaks.
I'll close this issue then.
Strictly speaking, defer()rered functions are not guaranteed to be called, so it is not the case that the default collector must call the deferred functions.
But users will expect they will be called on best-effort basis, which can also be practically important because some objects can be bound to resources other than memory (e.g. database connections) and should be properly closed, at least on best-effort basis.
So I'm in favor of calling all the remaining deferred functions when the last Handle is dropped. Currently I don't have any good implementation idea, though..
@jeehoonkang What is the reason that their being called isn't guaranteed? I presume it's a performance issue with guards and pinning? If calling them when the Collector itself is dropped, then maybe a better guarantee would be that they're guaranteed to be called before the Collector is dropped, but not necessarily any sooner. It retains the performance because they can still outlive a guard, but give stronger guarantees to reason about.
@joshlf Simply because Rust doesn't guarantee drop() is called. See http://huonw.github.io/blog/2016/04/memory-leaks-are-memory-safe/ for more details. In short, while drop() is not guaranteed to be called, it is called on best-effort basis. My argument is: so should we.
Except that, in Rust, the cases in which drop isn't guaranteed to be called are well-defined: if you have reference cycles or use mem::forget. By knowing the rules of whether drop will be called, you can still guarantee it.
@joshlf Do you have a reference for that? It'll be very useful to know exactly when drop will not be called!
Well drop is guaranteed to be called on any local variable when it goes out of scope. That's really all you need to know - everything else is done using data structures that use unsafe (e.g., Rc and Arc), so it's up to the author to document the behavior of their data structures. Even ManuallyDrop isn't special-cased by the compiler - it's just a union, which is unsafe (mem::forget is implemented using ManuallyDrop).