[daemon] ensure all outstanding futures are done before exit #4010
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4010 +/- ##
==========================================
+ Coverage 89.34% 89.53% +0.19%
==========================================
Files 260 260
Lines 14738 14741 +3
==========================================
+ Hits 13167 13198 +31
+ Misses 1571 1543 -28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| */ | ||
| for (auto& [_, watcher] : async_future_watchers) | ||
| { | ||
| watcher->waitForFinished(); |
There was a problem hiding this comment.
This line is not always going to be hit by the unit tests since it depends on the timing. The unit test ensure_that_on_restart_future_completes should ensure that there's always a watcher alive in the destructor.
5865dff to
450087e
Compare
| * object, which makes it troublesome to do this in the AsyncPeriodicDownloadTask | ||
| * destructor. | ||
| */ | ||
| update_manifests_all_task.shutdown(); |
There was a problem hiding this comment.
This fixes the notorious sporadic CI daemon unit test crash.
d5e72ec to
0802da0
Compare
|
Caught another crash related to this. It happens when the daemon constructor throws during a VM construction: |
@xmkg For reference only, one trick we can use is a private constructor which does not throw before the main body of the |
Hi @georgeliao, thanks for chiming in -- excellent idea! It's a nice trick, and could work in our scenario. I tried it and it works like a charm :) |
The daemon object hosts to QFuture and QFutureWatcher objects, which may be still active while the daemon is being destructed. This patch ensures that the destructor call waits for all outstanding futures, and all the raised signals during the lifetime of the futures are delivered to the recipients. Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
Added the `ensure_that_on_restart_future_completes` test case to validate that the daemon destructor waits for the future completion and also delivers the pending signals. Also suppressed a few mock naggings. Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
AsyncPeriodicDownloadTask also carries a QFuture/QFutureWatcher pair. Hence, update_manifests_all_task object also needs to be properly shutdown before the daemon destructor exits. Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
0802da0 to
90099bd
Compare
| .WillOnce(Return(mp::VirtualMachine::State::stopped)); | ||
| EXPECT_CALL(*mock_vm, start).Times(1); | ||
|
|
||
| mpt::Signal signal; |
There was a problem hiding this comment.
Nice interplay of the mpt::Signal in the main thread and the side thread.
|
@xmkg |
|
@georgeliao thanks for the review! Right now, I'm also looking into moving the future cleanup calls into their respective types/classes, but that could be handled in a separate PR -- maybe in the "handle cleanup properly when the daemon constructor throws" PR. Meanwhile, I'm happy to land this one. @levkropp this one's all yours now. Let me know if you want me to make any changes or provide an explanation. |
|
@georgeliao @levkropp thanks folks! |
The daemon object hosts to QFuture and QFutureWatcher objects, which may be still active while the daemon is being destructed.
This patch ensures that the destructor call waits for all outstanding futures, and all the raised signals during the lifetime of the futures are delivered to the recipients.
MULTI-1911