Wednesday, 27 September 2023
pumphausRecently, I’ve stumbled across some behavior of C++ lambda captures that has, initially, made absolutely no sense to me. Apparently, I wasn’t alone with this, because it has resulted in a memory leak in QtFuture::whenAll() and QtFuture::whenAny() (now fixed; more on that further down).
I find the corner cases of C++ quite interesting, so I wanted to share this. Luckily, we can discuss this without getting knee-deep into the internals of QtFuture. So, without further ado:
Time for an example
Consider this (godbolt):
#include <iostream>
#include <functional>
#include <memory>
#include <cassert>
#include <vector>
struct Job
{
template<class T>
Job(T &&func) : func(std::forward<T>(func)) {}
void run() { func(); hasRun = true; }
std::function<void()> func;
bool hasRun = false;
};
std::vector<Job> jobs;
template<class T>
void enqueueJob(T &&func)
{
jobs.emplace_back([func=std::forward<T>(func)]() mutable {
std::cout << "Starting job..." << std::endl;
// Move func to ensure that it is destroyed after running
auto fn = std::move(func);
fn();
std::cout << "Job finished." << std::endl;
});
}
int main()
{
struct Data {};
std::weak_ptr<Data> observer;
{
auto context = std::make_shared<Data>();
observer = context;
enqueueJob([context] {
std::cout << "Running..." << std::endl;
});
}
for (auto &job : jobs) {
job.run();
}
assert((observer.use_count() == 0)
&& "There's still shared data left!");
}
Output:
Starting job... Running... Job finished.
The code is fairly straight forward. There’s a list of jobs to which we can be append with enqueueJob(). enqueueJob() wraps the passed callable with some debug output and ensures that it is destroyed after calling it. The Job objects themselves are kept around a little longer; we can imagine doing something with them, even though the jobs have already been run.
In main(), we enqueue a job that captures some shared state Data, run all jobs, and finally assert that the shared Data has been destroyed. So far, so good.
Now you might have some issues with the code. Apart from the structure, which, arguably, is a little forced, you might think “context is never modified, so it should be const!”. And you’re right, that would be better. So let’s change it (godbolt):
--- old
+++ new
@@ -34,7 +34,7 @@
struct Data {};
std::weak_ptr<Data> observer;
{
- auto context = std::make_shared<Data>();
+ const auto context = std::make_shared<Data>();
observer = context;
enqueueJob([context] {
std::cout << "Running..." << std::endl;
Looks like a trivial change, right? But when we run it, the assertion fails now!
int main(): Assertion `(observer.use_count() == 0) && "There's still shared data left!"' failed.
How can this be? We’ve just declared a variable const that isn’t even used once! This does not seem to make any sense.
But it gets better: we can fix this by adding what looks like a no-op (godbolt):
--- old
+++ new
@@ -34,9 +34,9 @@
struct Data {};
std::weak_ptr<Data> observer;
{
- auto context = std::make_shared<Data>();
+ const auto context = std::make_shared<Data>();
observer = context;
- enqueueJob([context] {
+ enqueueJob([context=context] {
std::cout << "Running..." << std::endl;
});
}Wait, what? We just have to tell the compiler that we really want to capture context by the name context – and then it will correctly destroy the shared data? Would this be an application for the really keyword? Whatever it is, it works; you can check it on godbolt yourself.
When I first stumbled across this behavior, I just couldn’t wrap my head around it. I was about to think “compiler bug”, as unlikely as that may be. But GCC and Clang both behave like this, so it’s pretty much guaranteed not to be a compiler bug.
So, after combing through the interwebs, I’ve found this StackOverflow answer that gives the right hint: [context] is not the same as [context=context]! The latter drops cv qualifiers while the former does not! Quoting cppreference.com:
Those data members that correspond to captures without initializers are direct-initialized when the lambda-expression is evaluated. Those that correspond to captures with initializers are initialized as the initializer requires (could be copy- or direct-initialization). If an array is captured, array elements are direct-initialized in increasing index order. The order in which the data members are initialized is the order in which they are declared (which is unspecified).
https://en.cppreference.com/w/cpp/language/lambda
So [context] will direct-initialize the corresponding data member, whereas [context=context] (in this case) does copy-initialization! In terms of code this means:
[context]is equivalent todecltype(context) captured_context{context};, i.e.const std::shared_ptr<Data> captured_context{context};[context=context]is equivalent toauto capture_context = context;, i.e.std::shared_ptr<Data> captured_context = context;
Good, so writing [context=context] actually drops the const qualifier on the captured variable! Thus, for the lambda, it is equivalent to not having written it in the first place and using direct-initialization.
But why does this even matter? Why do we leak references to the shared_ptr<Data> if the captured variable is const? We only ever std::move() or std::forward() the lambda, right up to the place where we invoke it. After that, it goes out of scope, and all captures should be destroyed as well. Right?
Nearly. Let’s think about the compiler generates for us when we write a lambda. For the direct-initialization capture (i.e. [context]() {}), the compiler roughly generates something like this:
struct lambda
{
const std::shared_ptr<Data> context;
// ...
};This is what we want to to std::move() around. But it contains a const data member, and that cannot be moved from (it’s const after all)! So even with std::move(), there’s still a part of the lambda that lingers, keeping a reference to context. In the example above, the lingering part is in func, the capture of the wrapper lambda created in enqueueJob(). We move from func to ensure that all captures are destroyed when the it goes out of scope. But for the const std::shared_ptr<Data> context, which is hidden inside func, this does not work. It keeps holding the reference. The wrapper lambda itself would have to be destroyed for the reference count to drop to zero.
However, we keep the already-finished jobs around, so this never happens. The assertion fails.
How does this matter for Qt?
QtFuture::whenAll() and whenAny() create a shared_ptr to a Context struct and capture that in two lambdas used as continuations on a QFuture. Upon completion, the Context stores a reference to the QFuture. Similar to what we have seen above, continuations attached to QFuture are also wrapped by another lambda before being stored. When invoked, the “inner” lambda is supposed to be destroyed, while the outer (wrapper) one is kept alive.
In contrast to our example, the QFuture situation had created an actual memory leak, though (QTBUG-116731): The “inner” continuation references the Context, which references the QFuture, which again references the continuation lambda, referencing the Context. The “inner” continuation could not be std::move()d and destroyed after invocation, because the std::shared_ptr data member was const. This had created a reference cycle, leaking memory. I’ve also cooked this more complex case down to a small example (godbolt).
The patch for all of this is very small. As in the example, it simply consists of making the capture [context=context]. It’s included in the upcoming Qt 6.6.0.
Bottom line
I seriously didn’t expect there to be these differences in initialization of by-value lambda captures. Why doesn’t [context] alone also do direct- or copy-initialization, i.e. be exactly the same as [context=context]? That would be the sane thing to do, I think. I guess there is some reasoning for this; but I couldn’t find it (yet). It probably also doesn’t make a difference in the vast majority of cases.
In any case, I liked hunting this one down and getting to know another one of those dark corners of the C++ spec. So it’s not all bad 
.
@redstrate:pyra.sh
