Here is a test case that drops a Downloader on the floor without calling finishDownload():
TEST_F(DownloadTest, abandoned)
{
set_provider(unique_ptr<provider::ProviderBase>(new MockProvider()));
Item root;
{
unique_ptr<ItemJob> j(acc_.get("root_id"));
QSignalSpy spy(j.get(), &ItemJob::statusChanged);
spy.wait(SIGNAL_WAIT_TIME);
root = j->item();
}
Item child;
{
unique_ptr<ItemJob> j(acc_.get("child_id"));
QSignalSpy spy(j.get(), &ItemJob::statusChanged);
spy.wait(SIGNAL_WAIT_TIME);
child = j->item();
}
unique_ptr<Downloader> downloader(child.createDownloader());
EXPECT_TRUE(downloader->isValid());
{
QSignalSpy spy(downloader.get(), &Downloader::statusChanged);
spy.wait(SIGNAL_WAIT_TIME);
auto arg = spy.takeFirst();
EXPECT_EQ(Downloader::Status::Ready, qvariant_cast<Downloader::Status>(arg.at(0)));
}
}
Valgrind complains about this:
==43641== 1,385 (272 direct, 1,113 indirect) bytes in 1 blocks are definitely lost in loss record 216 of 227
==43641== at 0x4C2D1AF: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==43641== by 0x4FDE17C: boost::promise<void>::promise() (future.hpp:2570)
==43641== by 0x4FDEB64: boost::make_ready_future() (future.hpp:4175)
==43641== by 0x4FDDC04: unity::storage::provider::internal::DownloadJobImpl::cancel(unity::storage::provider::DownloadJob&) (DownloadJobImpl.cpp:161)
==43641== by 0x4FF47E4: void unity::storage::provider::internal::PendingJobs::cancel_job<unity::storage::provider::DownloadJob>(std::shared_ptr<unity::storage::provider::DownloadJob> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (PendingJobs.cpp:181)
==43641== by 0x4FF0B25: unity::storage::provider::internal::PendingJobs::~PendingJobs() (PendingJobs.cpp:55)
==43641== by 0x4FF0DFB: unity::storage::provider::internal::PendingJobs::~PendingJobs() (PendingJobs.cpp:61)
==43641== by 0x4F95CB3: std::default_delete<unity::storage::provider::internal::PendingJobs>::operator()(unity::storage::provider::internal::PendingJobs*) const (unique_ptr.h:76)
==43641== by 0x4F94BE7: std::unique_ptr<unity::storage::provider::internal::PendingJobs, std::default_delete<unity::storage::provider::internal::PendingJobs> >::~unique_ptr() (unique_ptr.h:236)
==43641== by 0x4F908EF: unity::storage::provider::internal::AccountData::~AccountData() (AccountData.h:51)
==43641== by 0x505BACA: void __gnu_cxx::new_allocator<unity::storage::provider::internal::AccountData>::destroy<unity::storage::provider::internal::AccountData>(unity::storage::provider::internal::AccountData*) (new_allocator.h:124)
==43641== by 0x505B9CE: void std::allocator_traits<std::allocator<unity::storage::provider::internal::AccountData> >::destroy<unity::storage::provider::internal::AccountData>(std::allocator<unity::storage::provider::internal::AccountData>&, unity::storage::provider::internal::AccountData*) (alloc_traits.h:467)
Before classifying this as a definite leak, it'd be good to work out some way to return control to the event loop for some period after this clean-up.
Cancelling a job is potentially an asynchronous operation, so the future returned by cancel() needs to be kept alive until the future is ready. We wait for this to happen by attaching a continuation to the future that will be called later in the event loop.