mirror of
https://invent.kde.org/network/kdeconnect-kde.git
synced 2026-10-14 23:49:20 +02:00
## Summary When the daemon's SMS conversation cache does not contain the target thread (e.g. after a device reconnect, or before the SMS app has been opened for this session), `replyToConversation()` used to log a warning and silently drop the user's message. This MR makes the daemon request the missing conversation from the phone, wait for it, and then actually send the queued message on the same call. BUG: 517659 ## Test Plan ### Setup - KDE Connect paired with Android phone (SMS plugin enabled on Android). - Desktop daemon (`kdeconnectd`) and `kdeconnect-sms`. ### Cache-hit (happy path) 1. Open SMS app, let conversation list populate. 2. Send a message in any existing conversation. 3. Verify the message is delivered. ### Cache-miss (the fix) 1. Kill the daemon (`killall kdeconnectd`). 2. Launch both daemon and `kdeconnect-sms` in quick succession. 3. Attempt to send a message to a known conversation before the conversation list finishes loading. 4. Verify the message is delivered (with a small delay while the phone responds). ### Concurrent wait (QReadWriteLock refactor) 1. Open a conversation and scroll up rapidly to trigger `RequestConversationWorker::updateConversation()`. 2. While messages are still loading, send a new message. 3. Both threads call `updateConversation()` on the same ID; verify no deadlock, crash, or dropped send. ## Implementation Notes ### Why a worker thread on cache miss? `updateConversation()` blocks until the phone's reply arrives via `addMessages()`. `addMessages()` is delivered on the main thread, so blocking `replyToConversation()` (also called on the main thread) would deadlock. The worker thread pattern preserves main-thread responsiveness. ### Why QThread::create instead of QObject + moveToThread? The existing `RequestConversationWorker` uses `QObject + moveToThread` because it needs to emit signals back to the main thread. Our worker is one-shot and doesn't need to communicate back, so `QThread::create` is sufficient and avoids introducing another header + cpp pair. ### Why QReadWriteLock? The old `updateConversation()` logic refused to let more than one thread wait for the same conversation (it returned early). With the new cache-miss worker, a second thread could reasonably want to wait alongside an in-flight request (for example, the user triggers a scroll-to-load-more while the first-send worker is still waiting). Switching to `QReadWriteLock` lets multiple threads share the wait on a read lock while still serialising the request-initiation under a write lock. ### Known limitations - If the phone never responds (disconnected, doesn't have the conversation), the worker thread blocks until daemon shutdown. Same pre-existing behavior as `RequestConversationWorker`. Could be addressed with a timeout in a follow-up. - A retry loop (`maxRetries = 3`) guards against BUG 400488 style empty responses from the phone. ## Disclosure This MR was developed with AI coding assistant support (Windsurf / Claude). All code was reviewed by me, tested end-to-end on Linux against an Android device, and iterated based on review feedback from @sredman in the previous revision.
264 lines
11 KiB
C++
264 lines
11 KiB
C++
/**
|
|
* SPDX-FileCopyrightText: 2018 Simon Redman <simon@ergotech.com>
|
|
*
|
|
* SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
|
|
*/
|
|
|
|
#include "conversationsdbusinterface.h"
|
|
#include "dbusinterfaces/dbusinterfaces.h"
|
|
#include "models/conversationmessage.h"
|
|
|
|
#include "requestconversationworker.h"
|
|
|
|
#include <QDBusConnection>
|
|
|
|
#include <core/device.h>
|
|
#include <core/kdeconnectplugin.h>
|
|
|
|
#include "kdeconnect_conversations_debug.h"
|
|
|
|
ConversationsDbusInterface::ConversationsDbusInterface(KdeConnectPlugin *plugin)
|
|
: QDBusAbstractAdaptor(const_cast<Device *>(plugin->device()))
|
|
, m_device(plugin->device()->id())
|
|
, m_lastId(0)
|
|
, m_smsInterface(m_device)
|
|
{
|
|
ConversationMessage::registerDbusType();
|
|
}
|
|
|
|
ConversationsDbusInterface::~ConversationsDbusInterface()
|
|
{
|
|
// Wake all threads which were waiting for a reply from this interface
|
|
// This might result in some noise on dbus, but it's better than leaking a bunch of resources!
|
|
waitingForMessagesLock.lockForWrite();
|
|
conversationsWaitingForMessages.clear();
|
|
waitingForMessages.wakeAll();
|
|
waitingForMessagesLock.unlock();
|
|
}
|
|
|
|
QVariantList ConversationsDbusInterface::activeConversations()
|
|
{
|
|
QList<QVariant> toReturn;
|
|
toReturn.reserve(m_conversations.size());
|
|
|
|
for (auto it = m_conversations.cbegin(); it != m_conversations.cend(); ++it) {
|
|
const auto &conversation = it.value().values();
|
|
if (conversation.isEmpty()) {
|
|
// This should really never happen because we create a conversation at the same time
|
|
// as adding a message, but better safe than sorry
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "Conversation with ID" << it.key() << "is unexpectedly empty";
|
|
break;
|
|
}
|
|
const QVariant &message = QVariant::fromValue<ConversationMessage>(*conversation.crbegin());
|
|
toReturn.append(message);
|
|
}
|
|
|
|
return toReturn;
|
|
}
|
|
|
|
void ConversationsDbusInterface::requestConversation(const qint64 &conversationID, int start, int end)
|
|
{
|
|
if (start < 0 || end < 0) {
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "requestConversation"
|
|
<< "Start and end must be >= 0";
|
|
return;
|
|
}
|
|
|
|
if (end - start < 0) {
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "requestConversation"
|
|
<< "Start must be before end";
|
|
return;
|
|
}
|
|
|
|
RequestConversationWorker *worker = new RequestConversationWorker(conversationID, start, end, this);
|
|
connect(worker, &RequestConversationWorker::conversationMessageRead, this, &ConversationsDbusInterface::conversationUpdated, Qt::QueuedConnection);
|
|
worker->work();
|
|
}
|
|
|
|
void ConversationsDbusInterface::requestAttachmentFile(const qint64 &partID, const QString &uniqueIdentifier)
|
|
{
|
|
m_smsInterface.getAttachment(partID, uniqueIdentifier);
|
|
}
|
|
|
|
void ConversationsDbusInterface::addMessages(const QList<ConversationMessage> &messages)
|
|
{
|
|
QSet<qint64> updatedConversationIDs;
|
|
|
|
for (const auto &message : messages) {
|
|
const qint32 &threadId = message.threadID();
|
|
|
|
// We might discover that there are no new messages in this conversation, thus calling it
|
|
// "updated" might turn out to be a bit misleading
|
|
// However, we need to report it as updated regardless, for the case where we have already
|
|
// cached every message of the conversation but we have received a request for more, otherwise
|
|
// we will never respond to that request
|
|
updatedConversationIDs.insert(message.threadID());
|
|
|
|
if (m_known_messages[threadId].contains(message.uID())) {
|
|
// This message has already been processed. Don't do anything.
|
|
continue;
|
|
}
|
|
|
|
// Store the Message in the list corresponding to its thread
|
|
bool newConversation = !m_conversations.contains(threadId);
|
|
const auto &threadPosition = m_conversations[threadId].insert(message.date(), message);
|
|
m_known_messages[threadId].insert(message.uID());
|
|
|
|
// If this message was inserted at the end of the list, it is the latest message in the conversation
|
|
const bool latestMessage = std::distance(threadPosition, m_conversations[threadId].end()) == 1;
|
|
|
|
// Tell the world about what just happened
|
|
if (newConversation) {
|
|
Q_EMIT conversationCreated(QDBusVariant(QVariant::fromValue(message)));
|
|
} else if (latestMessage) {
|
|
Q_EMIT conversationUpdated(QDBusVariant(QVariant::fromValue(message)));
|
|
}
|
|
}
|
|
|
|
// It feels bad to go through the set of updated conversations again,
|
|
// but also there are not many times that updatedConversationIDs will be more than one
|
|
for (qint64 conversationID : updatedConversationIDs) {
|
|
quint64 numMessages = m_known_messages[conversationID].size();
|
|
Q_EMIT conversationLoaded(conversationID, numMessages);
|
|
}
|
|
|
|
waitingForMessagesLock.lockForWrite();
|
|
// Remove the waiting flag for all conversations which we just processed
|
|
conversationsWaitingForMessages.subtract(updatedConversationIDs);
|
|
waitingForMessages.wakeAll();
|
|
waitingForMessagesLock.unlock();
|
|
}
|
|
|
|
void ConversationsDbusInterface::removeMessage(const QString &internalId)
|
|
{
|
|
// TODO: Delete the specified message from our internal structures
|
|
Q_UNUSED(internalId);
|
|
}
|
|
|
|
QList<ConversationMessage> ConversationsDbusInterface::getConversation(const qint64 &conversationID) const
|
|
{
|
|
return m_conversations.value(conversationID).values();
|
|
}
|
|
|
|
void ConversationsDbusInterface::updateConversation(const qint64 &conversationID)
|
|
{
|
|
// Allow multiple threads to wait for the same conversation. Readers wait
|
|
// alongside an existing request; only the first thread initiates the fetch.
|
|
waitingForMessagesLock.lockForRead();
|
|
if (conversationsWaitingForMessages.contains(conversationID)) {
|
|
qCDebug(KDECONNECT_CONVERSATIONS) << "Joining existing wait for conversationID" << conversationID;
|
|
while (conversationsWaitingForMessages.contains(conversationID)) {
|
|
waitingForMessages.wait(&waitingForMessagesLock);
|
|
}
|
|
waitingForMessagesLock.unlock();
|
|
return;
|
|
}
|
|
waitingForMessagesLock.unlock();
|
|
|
|
// Nobody is waiting yet. Take write lock to initiate the request.
|
|
waitingForMessagesLock.lockForWrite();
|
|
// Double-check: another thread may have started between our read-unlock and write-lock.
|
|
if (conversationsWaitingForMessages.contains(conversationID)) {
|
|
waitingForMessagesLock.unlock();
|
|
waitingForMessagesLock.lockForRead();
|
|
while (conversationsWaitingForMessages.contains(conversationID)) {
|
|
waitingForMessages.wait(&waitingForMessagesLock);
|
|
}
|
|
waitingForMessagesLock.unlock();
|
|
return;
|
|
}
|
|
|
|
qCDebug(KDECONNECT_CONVERSATIONS) << "Requesting conversation with ID" << conversationID << "from remote";
|
|
conversationsWaitingForMessages.insert(conversationID);
|
|
|
|
// Request a window of messages
|
|
qint64 rangeStartTimestamp;
|
|
qint64 numberToRequest;
|
|
if (m_conversations.contains(conversationID) && m_conversations[conversationID].count() > 0) {
|
|
rangeStartTimestamp = m_conversations[conversationID].first().date(); // Request starting from the oldest-available message
|
|
numberToRequest = m_conversations[conversationID].count(); // Request an increasing number of messages by requesting more equal to the amount we have
|
|
} else {
|
|
rangeStartTimestamp = -1; // Value < 0 indicates to return the newest messages
|
|
numberToRequest = MIN_NUMBER_TO_REQUEST; // Start off with a small batch
|
|
}
|
|
if (numberToRequest < MIN_NUMBER_TO_REQUEST) {
|
|
numberToRequest = MIN_NUMBER_TO_REQUEST;
|
|
}
|
|
m_smsInterface.requestConversation(conversationID, rangeStartTimestamp, numberToRequest);
|
|
|
|
while (conversationsWaitingForMessages.contains(conversationID)) {
|
|
waitingForMessages.wait(&waitingForMessagesLock);
|
|
}
|
|
waitingForMessagesLock.unlock();
|
|
}
|
|
|
|
void ConversationsDbusInterface::sendSmsForConversation(const ConversationMessage &referenceMessage,
|
|
const QString &messageText,
|
|
const QVariantList &attachmentUrls)
|
|
{
|
|
const QList<ConversationAddress> &addressList = referenceMessage.addresses();
|
|
QVariantList addresses;
|
|
for (const auto &address : addressList) {
|
|
addresses << QVariant::fromValue(address);
|
|
}
|
|
m_smsInterface.sendSms(addresses, messageText, attachmentUrls, referenceMessage.subID());
|
|
}
|
|
|
|
void ConversationsDbusInterface::replyToConversation(const qint64 &conversationID, const QString &message, const QVariantList &attachmentUrls)
|
|
{
|
|
const auto messagesList = m_conversations[conversationID];
|
|
if (!messagesList.isEmpty()) {
|
|
sendSmsForConversation(messagesList.first(), message, attachmentUrls);
|
|
return;
|
|
}
|
|
|
|
// Cache miss: fetch the conversation from the phone, then send (BUG 517659).
|
|
// The blocking wait must run on a worker thread so the main event loop can
|
|
// still deliver the phone's response via addMessages(). Using QThread::create
|
|
// here (rather than the QObject+moveToThread pattern of RequestConversationWorker)
|
|
// because this is a one-shot task that does not need to signal results back.
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "Conversation" << conversationID << "not in cache, requesting from phone";
|
|
|
|
QThread *thread = QThread::create([this, conversationID, message, attachmentUrls]() {
|
|
// Retry a few times as a safety net in case the phone responds before
|
|
// the cache is populated. The first request should normally succeed.
|
|
const int maxRetries = 3;
|
|
for (int attempt = 0; attempt < maxRetries && m_conversations[conversationID].isEmpty(); ++attempt) {
|
|
updateConversation(conversationID);
|
|
}
|
|
|
|
const auto updatedMessages = m_conversations[conversationID];
|
|
if (updatedMessages.isEmpty()) {
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "Conversation" << conversationID << "still empty after requesting from phone, cannot send";
|
|
return;
|
|
}
|
|
|
|
sendSmsForConversation(updatedMessages.first(), message, attachmentUrls);
|
|
});
|
|
connect(thread, &QThread::finished, thread, &QObject::deleteLater);
|
|
thread->start();
|
|
}
|
|
|
|
void ConversationsDbusInterface::sendWithoutConversation(const QVariantList &addresses, const QString &message, const QVariantList &attachmentUrls)
|
|
{
|
|
m_smsInterface.sendSms(addresses, message, attachmentUrls);
|
|
}
|
|
|
|
void ConversationsDbusInterface::requestAllConversationThreads()
|
|
{
|
|
// Prepare the list of conversations by requesting the first in every thread
|
|
m_smsInterface.requestAllConversations();
|
|
}
|
|
|
|
QString ConversationsDbusInterface::newId()
|
|
{
|
|
return QString::number(++m_lastId);
|
|
}
|
|
|
|
void ConversationsDbusInterface::attachmentDownloaded(const QString &filePath, const QString &fileName)
|
|
{
|
|
Q_EMIT attachmentReceived(filePath, fileName);
|
|
}
|
|
|
|
#include "moc_conversationsdbusinterface.cpp"
|