D16092: Make SmsPlugin::requestConversation synchronous and blocking

[Simon Redman]

sredman added a comment.


  In D16092#340672 <https://phabricator.kde.org/D16092#340672>, @apol wrote:
  
  > Are you sure this makes it blocking?  Where are we blocking?
  
  
  See the inline comments. Would you like these written in to the source code?
  
  The idea is per https://wiki.qt.io/Threads_Events_QObjects#Forcing_event_dispatching . It works because the event queue handles signals serially, and the Device object (which receives the packet from the network), the SmsPlugin object (which handles the packet and the app's message request) and the ConversationDbusInterface (which handles the dbus request) are all resident on the same thread, so all events are handled by the same thread. Reentering the thread's event queue allows the data coming from Android to be handled. Exiting the event queue after the message is handled means the data we need is ready.
  
  I tried to achieve the same thing using locks and multiple threads, but I could not make it work. The core issue is that I could not get the ConversationDbusInterface and the Device to be on separate threads AND have the dbus interface properly registered to the dbus. This means, regardless what happens, the dbus interface waiting would block the packet handling. QWaitCondition::wait() does not allow other events on the same thread to be handled, apparently 😢
  
  For proof, put a breakpoint (or debug print) on smsplugin.ccp lines 56, 91, and 103, then launch the SMS app and open a conversation. You should see that the daemon hits line 91 when the request comes in, then 56 when the phone replies with the messages, then 103 as the daemon returns the messages to the phone (via the ConversationDbusInterface, of course)
  
  For a less difficult test, open the messaging app before this patch and open a conversation. Notice that only one message shows *the first time* because that is all the conversation interface knows about. After the first time a conversation is opened, it will show 10 messages when a conversation is opened because they are in cache. With this patch, you will see 10 every time because the dbus interface waits for its cache to be populated before returning.

INLINE COMMENTS

> smsplugin.cpp:55
> +        bool success = handleBatchMessages(np);
> +        Q_EMIT messagesPacketHandled();
> +        return success;

Unblock - The awaited data is ready (hopefully...)

> smsplugin.cpp:92
> +    QEventLoop loop;
> +    connect(this, &SmsPlugin::messagesPacketHandled, &loop, &QEventLoop::quit);
> +    loop.exec();

Connect the signal that we have processed some new data to the new event loop's quit signal

> smsplugin.cpp:93
> +    connect(this, &SmsPlugin::messagesPacketHandled, &loop, &QEventLoop::quit);
> +    loop.exec();
> +

Reenter the thread's event loop. loop.exec() does not return until signaled to quit

REPOSITORY
  R224 KDE Connect

REVISION DETAIL
  https://phabricator.kde.org/D16092

To: sredman, #kde_connect
Cc: apol, kdeconnect, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20181010/846da95e/attachment.html>