Skip to content

deferred: don't delete self, to allow smart pointers to manage#48174

Merged
jkarneges merged 1 commit intomainfrom
jkarneges/deferred-no-delete-self
Mar 21, 2025
Merged

deferred: don't delete self, to allow smart pointers to manage#48174
jkarneges merged 1 commit intomainfrom
jkarneges/deferred-no-delete-self

Conversation

@jkarneges
Copy link
Copy Markdown
Member

@jkarneges jkarneges commented Mar 21, 2025

The Deferred class provides a generic interface for an async operation, signaling a QVariant result. Currently it deletes itself upon completion (literally with delete this), intended to enable a fire-and-forget developer experience when used along with QObject parenting. The idea is whether it completes or its parent goes away, it is automatically cleaned up, without the caller needing to explicitly track it. However, we are trying to avoid QObject parenting, preferring to use smart pointers instead, and self-destruction is incompatible with smart pointers.

In order to allow Deferred instances to be managed with smart pointers, this PR removes the self destruction and updates all the places Deferred is used to ensure instances are destroyed on completion or when the parent/owner goes away. In some places we were already using smart pointers to manage some instances, which was almost certainly wrong (possibly leading to double-deletions) and this PR fixes those cases too.

The resulting developer experience is not as nice but at least this fixes some wrongness and helps get us off QObject.


class RefreshWorker : public Deferred
{
Q_OBJECT
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this should be here since Deferred inherits from QObject, but probably not for much longer.

@jkarneges jkarneges merged commit c54e146 into main Mar 21, 2025
19 checks passed
@jkarneges jkarneges deleted the jkarneges/deferred-no-delete-self branch March 21, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants