Combined operator<<() isn't thread safe if used outside main thread

[vpicaver]

An example of this issue is if two futures being added to Combined(). If future1 finished before future2 has been added, this will complete() the Combined(). Even though in this use case, we want the code below to print out "Finished!" once both future1 and future2 are finished. If future1 is finished before future2 is added, Finished() will be prematurely, called.

auto combine = Combined() << future1 << future2;
combine.subscribe([]() { qDebug() << "Finished!"; }

I don't think this can be easily be fixed without api changes to how Combine() is handled. Perhaps it better to advocate using async on the main thread only. The deferred needs to have a proper context object to make this work correrctly. By default deferred is move to the main thread.

[jsravn]

I notice it's worse than just early completing. It can lead to segfaults as well. If the future completes early, the main thread can end up deleting the combined future while the worker thread is about to call deleteLater on it.

It seems because this line was added https://github.com/benlau/asyncfuture/blob/master/asyncfuture.h#L762 as part of #15. @vpicaver do you know why that change was made?

[vpicaver]

That line does look suspicious. We should probably comment that line out and see if the testcases pass.

[vpicaver]

That line forces the deferred to be moved to the main thread. The call to deleteLater needs an event loop to run correctly. It's not guaranteed that a QThread will have an event loop.

[jsravn]

That line forces the deferred to be moved to the main thread. The call to deleteLater needs an event loop to run correctly. It's not guaranteed that a QThread will have an event loop.

Okay that makes sense. But by moving to the main thread, it makes deferred completion very racy. Here is an example of a crash I saw:

==19==ERROR: AddressSanitizer: SEGV on unknown address 0x000000009d68 (pc 0x7f902c9f22c5 bp 0x7f9010bff630 sp 0x7f9010bff2d0 T21)
==19==The signal is caused by a READ memory access.
    #0 0x7f902c9f22c5 in QMetaObject::invokeMethod(QObject*, char const*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) (/usr/local/Qt-5.12.10/lib/libQt5Core.so.5+0x2a42c5) (BuildId: 6c6bbc437378040231fa29c85c0a99767e8385ad)
    #1 0x15a09c8 in QMetaObject::invokeMethod(QObject*, char const*, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) qt/include/QtCore/qobjectdefs.h:460:16
    #2 0x15a09c8 in AsyncFuture::Private::DeferredFuture<ContextAwareResult<void>>::decWeakRefCount() asyncfuture.h:681:9
    #3 0x159ff17 in AsyncFuture::Private::DeferredFuture<ContextAwareResult<void>>::create()::'lambda'(AsyncFuture::Private::DeferredFuture<ContextAwareResult<void>>*)::operator()(AsyncFuture::Private::DeferredFuture<ContextAwareResult<void>>*) const asyncfuture.h:704:15

If the future completes and the shared pointer goes out of scope around the same time, the main thread and deleter race to delete the object. It was not a major issue before because it kept the deferred in the same thread. You can reproduce this trivially adding a small pause before invokeMethod(this, "deleteLater").

Any thoughts on how to fix it?

[jsravn]

@vpicaver Created #44 to fix the crashes.

[benlau]

@vpicaver Hello

I would like to express my utmost gratitude to you for resolve the issues in this project. I've found that I don't have the time to continue the project's development or handling requests. Would you be interested in taking over the project? I will archive this repository and set a link to your repository. What do you think?

@jsravn I think you should submit the PR to vpicaver's repository?

[vpicaver]

Sure! Phi|ip Sent from Gmail Mobile

…

On Sat, Oct 14, 2023 at 8:23 AM Ben Lau ***@***.***> wrote: @vpicaver <https://github.com/vpicaver> Hello I would like to express my utmost gratitude to you for resolve the issues in this project. I've found that I don't have the time to continue the project's development or handling requests. Would you be interested in taking over the project? I will archive this repository and set a link to your repository. What do you think? @jsravn <https://github.com/jsravn> I think you should submit the PR to vpicaver's repository? — Reply to this email directly, view it on GitHub <#34 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKKDA6DZSOKUVXKFGLSYD3X7KVA5ANCNFSM4L47J22Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>