Skip to content
This repository has been archived by the owner on Dec 12, 2023. It is now read-only.

Fix DeferredFuture crashes by using QWeakPointer #44

Closed
wants to merge 1 commit into from

Conversation

jsravn
Copy link

@jsravn jsravn commented Oct 14, 2023

This replaces the custom ref counting with QWeakPointer. It fixes several issues with the code that lead to crashes.

In the original code there is an inherent race between the QSharedPointer<DeferredFuture>'s deleter and decWeakRefCount() being called in the various watch lambdas:

  1. If there is even a 1ms pause after line

    object->decStrongRef();
    it always crashes when there are watchers. This is because the strongRefCount is decremented, which means deferred can be deleted by a watcher lambda when called. The deleter then crashes on the next line as object is no longer valid.

  2. If there is a pause before

    QMetaObject::invokeMethod(this, "deleteLater");
    , it is possible for the deleteLater to be invoked by the shared pointer deleter, and the object deleted in the main thread. This then crashes as invokeMethod can't call deleteLater on a deleted object.

  3. Calling raw delete this is not thread safe (

    delete this;
    ), it should always be doing deleteLater in the presence of multiple threads calling deleteLater.

There is not really a good way to fix this with the existing approach. The best way I found is to use the QSharedPointer semantics to handle ordered deletion. We can maintain a weakRef internally which can be used to obtain a shared pointer reference whenever we need to create a new watcher.

In my testing, this leads to deferred being correctly deleted in all cases without any race condition.

@jsravn
Copy link
Author

jsravn commented Oct 14, 2023

@vpicaver appreciate if you could have a look. All unit tests pass.

@jsravn jsravn force-pushed the fix-deferred-crashes branch 2 times, most recently from 537fb9a to f465dc9 Compare October 14, 2023 14:23
This replaces the custom ref counting with QWeakPointer. It fixes
several issues with the code that lead to crashes.

In the original code there is an inherent race between the
QSharedPointer<DeferredFuture<T>>'s deleter and decWeakRefCount() being
called in the various watch lambdas:

1. If there is even a 1ms pause after line
   https://github.com/benlau/asyncfuture/blob/f2c773bc3d08c43c532d191be5f05617b3c9f088/asyncfuture.h#L712
   it always crashes when there are watchers. This is because the
   strongRefCount is decremented, which means deferred can be deleted
   by a watcher lambda when called. The deleter then crashes on the next
   line as object is no longer valid.

2. If there is a pause before
   https://github.com/benlau/asyncfuture/blob/f2c773bc3d08c43c532d191be5f05617b3c9f088/asyncfuture.h#L689,
   it is possible for the deleteLater to be invoked by the shared
   pointer deleter, and the object deleted in the main thread. This then
   crashes as invokeMethod can't call deleteLater on a deleted object.

3. Calling raw delete this is not thread safe
   (https://github.com/benlau/asyncfuture/blob/f2c773bc3d08c43c532d191be5f05617b3c9f088/asyncfuture.h#L691),
   it should always be doing deleteLater in the presence of multiple
   threads calling deleteLater.

There is not really a good way to fix this with the existing approach.
The best way I found is to use the QSharedPointer semantics to handle
ordered deletion. We can maintain a weakRef internally which can be used
to obtain a shared pointer reference whenever we need to create a new
watcher.

In my testing, this leads to deferred being correctly deleted in all
cases without any race condition.
@vpicaver
Copy link
Contributor

Do you mind moving this pull request onto vpicaver/asyncfuture?

@jsravn jsravn closed this Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants