From 0a82e68f86a3b4716ebada39c041a6c8673d2999 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 17 Oct 2019 09:59:37 +0900 Subject: Connection::joinRoom: make sure the room object is created early enough All direct slots connected to finished() will run before success() is even emitted; so create the room object in the earliest slot connected to finished(), rather than to success(). --- lib/connection.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 2c5bf574..25f1c3f6 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -534,10 +534,14 @@ JoinRoomJob* Connection::joinRoom(const QString& roomAlias, const QStringList& serverNames) { auto job = callApi(roomAlias, serverNames); - // Upon completion, ensure a room object in Join state is created but only - // if it's not already there due to a sync completing earlier. - connect(job, &JoinRoomJob::success, this, - [this, job] { provideRoom(job->roomId()); }); + // Upon completion, ensure a room object in Join state is created + // (or it might already be there due to a sync completing earlier). + // finished() is used here instead of success() to overtake clients + // that may add their own slots to finished(). + connect(job, &BaseJob::finished, this, [this, job] { + if (job->status().good()) + provideRoom(job->roomId()); + }); return job; } -- cgit v1.2.3 From 8b9207d5a04386957d8eab8dd251421eaaa7c0d2 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 20 Oct 2019 19:13:56 +0900 Subject: Qualify types in signals and Q_INVOKABLEs Because https://doc.qt.io/qt-5/moc.html#limitations . For direct connections that doesn't matter but it very much does for queued ones. Along with this DirectChatsMap and IgnoredUsersList have been moved from Connection:: to Quotient::. --- lib/connection.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 25f1c3f6..af85d066 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -385,7 +385,7 @@ void Connection::syncLoop(int timeout) syncLoopIteration(); // initial sync to start the loop } -QJsonObject toJson(const Connection::DirectChatsMap& directChats) +QJsonObject toJson(const DirectChatsMap& directChats) { QJsonObject json; for (auto it = directChats.begin(); it != directChats.end();) { @@ -1050,7 +1050,7 @@ QVector Connection::roomsWithTag(const QString& tagName) const return rooms; } -Connection::DirectChatsMap Connection::directChats() const +DirectChatsMap Connection::directChats() const { return d->directChats; } @@ -1117,7 +1117,7 @@ bool Connection::isIgnored(const User* user) const return ignoredUsers().contains(user->id()); } -Connection::IgnoredUsersList Connection::ignoredUsers() const +IgnoredUsersList Connection::ignoredUsers() const { const auto* event = d->unpackAccountData(); return event ? event->ignored_users() : IgnoredUsersList(); -- cgit v1.2.3 From 693beec0005bdd98732ad8b4ad760f9de4a9faee Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 15 Oct 2019 19:33:21 +0900 Subject: Connection: make syncLoop() reentrant ...in the sense that you can call it twice and expect the second invocation to be gracefully ignored rather than two loops conflicting with each other. --- lib/connection.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index af85d066..a7b1bee9 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -99,6 +99,7 @@ public: DirectChatsMap dcLocalAdditions; DirectChatsMap dcLocalRemovals; UnorderedMap accountData; + QMetaObject::Connection syncLoopConnection {}; int syncLoopTimeout = -1; GetCapabilitiesJob* capabilitiesJob = nullptr; @@ -380,9 +381,20 @@ void Connection::sync(int timeout) void Connection::syncLoop(int timeout) { - d->syncLoopTimeout = timeout; - connect(this, &Connection::syncDone, this, &Connection::syncLoopIteration); - syncLoopIteration(); // initial sync to start the loop + if (d->syncLoopConnection && d->syncLoopTimeout == timeout) { + qCInfo(MAIN) << "Attempt to run sync loop but there's one already " + "running; nothing will be done"; + return; + } + std::swap(d->syncLoopTimeout, timeout); + if (d->syncLoopConnection) { + qCWarning(MAIN) << "Overriding timeout of the running sync loop from" + << timeout << "to" << d->syncLoopTimeout; + } else { + d->syncLoopConnection = connect(this, &Connection::syncDone, + this, &Connection::syncLoopIteration); + syncLoopIteration(); // initial sync to start the loop + } } QJsonObject toJson(const DirectChatsMap& directChats) @@ -514,8 +526,7 @@ void Connection::onSyncSuccess(SyncData&& data, bool fromCache) void Connection::stopSync() { // If there's a sync loop, break it - disconnect(this, &Connection::syncDone, this, - &Connection::syncLoopIteration); + disconnect(d->syncLoopConnection); if (d->syncJob) // If there's an ongoing sync job, stop it too { d->syncJob->abandon(); -- cgit v1.2.3 From ff77965cb92e94061345e4e955f1d7289de2597a Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 21 Oct 2019 15:58:12 +0900 Subject: Connection: record sync timeout; suspend sync before logout This is mostly internal but clients may see fewer spurious sync failures upon logging out. --- lib/connection.cpp | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index a7b1bee9..3d617733 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -100,7 +100,7 @@ public: DirectChatsMap dcLocalRemovals; UnorderedMap accountData; QMetaObject::Connection syncLoopConnection {}; - int syncLoopTimeout = -1; + int syncTimeout = -1; GetCapabilitiesJob* capabilitiesJob = nullptr; GetCapabilitiesJob::Capabilities capabilities; @@ -260,8 +260,6 @@ void Connection::doConnectToServer(const QString& user, const QString& password, }); } -void Connection::syncLoopIteration() { sync(d->syncLoopTimeout); } - void Connection::connectWithToken(const QString& userId, const QString& accessToken, const QString& deviceId) @@ -336,22 +334,34 @@ void Connection::checkAndConnect(const QString& userId, void Connection::logout() { - auto job = callApi(); - connect(job, &LogoutJob::finished, this, [job, this] { + // If there's an ongoing sync job, stop it but don't break the sync loop yet + const auto syncWasRunning = bool(d->syncJob); + if (syncWasRunning) + { + d->syncJob->abandon(); + d->syncJob = nullptr; + } + const auto* job = callApi(); + connect(job, &LogoutJob::finished, this, [this, job, syncWasRunning] { if (job->status().good() || job->error() == BaseJob::ContentAccessError) { - stopSync(); + if (d->syncLoopConnection) + disconnect(d->syncLoopConnection); d->data->setToken({}); emit stateChanged(); emit loggedOut(); - } + } else if (syncWasRunning) + syncLoopIteration(); // Resume sync loop (or a single sync) }); } void Connection::sync(int timeout) { - if (d->syncJob) + if (d->syncJob) { + qCInfo(MAIN) << d->syncJob << "is already running"; return; + } + d->syncTimeout = timeout; Filter filter; filter.room->timeline->limit = 100; filter.room->state->lazyLoadMembers = d->lazyLoading; @@ -381,22 +391,25 @@ void Connection::sync(int timeout) void Connection::syncLoop(int timeout) { - if (d->syncLoopConnection && d->syncLoopTimeout == timeout) { + if (d->syncLoopConnection && d->syncTimeout == timeout) { qCInfo(MAIN) << "Attempt to run sync loop but there's one already " "running; nothing will be done"; return; } - std::swap(d->syncLoopTimeout, timeout); + std::swap(d->syncTimeout, timeout); if (d->syncLoopConnection) { - qCWarning(MAIN) << "Overriding timeout of the running sync loop from" - << timeout << "to" << d->syncLoopTimeout; + qCInfo(MAIN) << "Timeout for next syncs changed from" + << timeout << "to" << d->syncTimeout; } else { d->syncLoopConnection = connect(this, &Connection::syncDone, - this, &Connection::syncLoopIteration); + this, &Connection::syncLoopIteration, + Qt::QueuedConnection); syncLoopIteration(); // initial sync to start the loop } } +void Connection::syncLoopIteration() { sync(d->syncTimeout); } + QJsonObject toJson(const DirectChatsMap& directChats) { QJsonObject json; -- cgit v1.2.3 From 60bb1cf942ad0815dcf42cbfe8acd1e076d848cf Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 29 Oct 2019 22:04:40 +0900 Subject: Derive Omittable<> from std::optional<> That breaks API all over the place but: 1. The fixes are trivial. 2. More of std:: is used instead of home-baking the same stuff. --- lib/connection.cpp | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 3d617733..47c643b0 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -277,16 +277,16 @@ void Connection::reloadCapabilities() else if (d->capabilitiesJob->error() == BaseJob::IncorrectRequestError) qCDebug(MAIN) << "Server doesn't support /capabilities"; - if (d->capabilities.roomVersions.omitted()) { + if (!d->capabilities.roomVersions) { qCWarning(MAIN) << "Pinning supported room version to 1"; - d->capabilities.roomVersions = { "1", { { "1", "stable" } } }; + d->capabilities.roomVersions.emplace({ "1", { { "1", "stable" } } }); } else { qCDebug(MAIN) << "Room versions:" << defaultRoomVersion() << "is default, full list:" << availableRoomVersions(); } - Q_ASSERT(!d->capabilities.roomVersions.omitted()); + Q_ASSERT(d->capabilities.roomVersions.has_value()); emit capabilitiesLoaded(); - for (auto* r : d->roomMap) + for (auto* r : qAsConst(d->roomMap)) r->checkVersion(); }); } @@ -295,7 +295,7 @@ bool Connection::loadingCapabilities() const { // (Ab)use the fact that room versions cannot be omitted after // the capabilities have been loaded (see reloadCapabilities() above). - return d->capabilities.roomVersions.omitted(); + return !d->capabilities.roomVersions; } void Connection::Private::connectWithToken(const QString& userId, @@ -363,8 +363,8 @@ void Connection::sync(int timeout) d->syncTimeout = timeout; Filter filter; - filter.room->timeline->limit = 100; - filter.room->state->lazyLoadMembers = d->lazyLoading; + filter.room.edit().timeline.edit().limit.emplace(100); + filter.room.edit().state.edit().lazyLoadMembers.emplace(d->lazyLoading); auto job = d->syncJob = callApi(BackgroundRequest, d->data->lastEvent(), filter, timeout); @@ -446,7 +446,7 @@ void Connection::onSyncSuccess(SyncData&& data, bool fromCache) r->updateData(std::move(roomData), fromCache); if (d->firstTimeRooms.removeOne(r)) { emit loadedRoomState(r); - if (!d->capabilities.roomVersions.omitted()) + if (d->capabilities.roomVersions) r->checkVersion(); // Otherwise, the version will be checked in reloadCapabilities() } @@ -1191,7 +1191,7 @@ Room* Connection::provideRoom(const QString& id, Omittable joinState) // and emit a signal. For Invite and Join, there's no such problem. if (room->joinState() == joinState && joinState != JoinState::Leave) return room; - } else if (joinState.omitted()) { + } else if (!joinState) { // No Join and Leave, maybe Invite? room = d->roomMap.value({ id, true }, nullptr); if (room) @@ -1200,9 +1200,7 @@ Room* Connection::provideRoom(const QString& id, Omittable joinState) } if (!room) { - room = roomFactory()(this, id, - joinState.omitted() ? JoinState::Join - : joinState.value()); + room = roomFactory()(this, id, joinState.value_or(JoinState::Join)); if (!room) { qCCritical(MAIN) << "Failed to create a room" << id; return nullptr; @@ -1213,7 +1211,7 @@ Room* Connection::provideRoom(const QString& id, Omittable joinState) &Connection::aboutToDeleteRoom); emit newRoom(room); } - if (joinState.omitted()) + if (!joinState) return room; if (joinState == JoinState::Invite) { @@ -1431,13 +1429,13 @@ const QString Connection::SupportedRoomVersion::StableTag = QString Connection::defaultRoomVersion() const { - Q_ASSERT(!d->capabilities.roomVersions.omitted()); + Q_ASSERT(d->capabilities.roomVersions.has_value()); return d->capabilities.roomVersions->defaultVersion; } QStringList Connection::stableRoomVersions() const { - Q_ASSERT(!d->capabilities.roomVersions.omitted()); + Q_ASSERT(d->capabilities.roomVersions.has_value()); QStringList l; const auto& allVersions = d->capabilities.roomVersions->available; for (auto it = allVersions.begin(); it != allVersions.end(); ++it) @@ -1457,7 +1455,7 @@ inline bool roomVersionLess(const Connection::SupportedRoomVersion& v1, QVector Connection::availableRoomVersions() const { - Q_ASSERT(!d->capabilities.roomVersions.omitted()); + Q_ASSERT(d->capabilities.roomVersions.has_value()); QVector result; result.reserve(d->capabilities.roomVersions->available.size()); for (auto it = d->capabilities.roomVersions->available.begin(); -- cgit v1.2.3 From edbbc2bc77599ead0e14bc08cdddda10d1c5f305 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 31 Oct 2019 12:48:49 +0900 Subject: Omittable: get rid of value() Xcode 10 doesn't have it, and value() is not quite fitting mostly-exceptionless Quotient anyway. --- lib/connection.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 47c643b0..998b45d1 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -1182,7 +1182,7 @@ Room* Connection::provideRoom(const QString& id, Omittable joinState) // TODO: This whole function is a strong case for a RoomManager class. Q_ASSERT_X(!id.isEmpty(), __FUNCTION__, "Empty room id"); - // If joinState.omitted(), all joinState == comparisons below are false. + // If joinState is empty, all joinState == comparisons below are false. const auto roomKey = qMakePair(id, joinState == JoinState::Invite); auto* room = d->roomMap.value(roomKey, nullptr); if (room) { @@ -1214,17 +1214,17 @@ Room* Connection::provideRoom(const QString& id, Omittable joinState) if (!joinState) return room; - if (joinState == JoinState::Invite) { + if (*joinState == JoinState::Invite) { // prev is either Leave or nullptr auto* prev = d->roomMap.value({ id, false }, nullptr); emit invitedRoom(room, prev); } else { - room->setJoinState(joinState.value()); + room->setJoinState(*joinState); // Preempt the Invite room (if any) with a room in Join/Leave state. auto* prevInvite = d->roomMap.take({ id, true }); - if (joinState == JoinState::Join) + if (*joinState == JoinState::Join) emit joinedRoom(room, prevInvite); - else if (joinState == JoinState::Leave) + else if (*joinState == JoinState::Leave) emit leftRoom(room, prevInvite); if (prevInvite) { const auto dcUsers = prevInvite->directChatUsers(); -- cgit v1.2.3 From ae5c2aa9acce500e2ad94c6781843c02310dbab3 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 1 Nov 2019 20:47:07 +0900 Subject: Omittable: Add direct-list-initialising operator=; document See the change in connection.cpp for the example of usage. Also: removed static_asserts: the first one is provided by std::optional, and the second one is only relevant to edit(). --- lib/connection.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 998b45d1..5ee7e84e 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -279,7 +279,7 @@ void Connection::reloadCapabilities() if (!d->capabilities.roomVersions) { qCWarning(MAIN) << "Pinning supported room version to 1"; - d->capabilities.roomVersions.emplace({ "1", { { "1", "stable" } } }); + d->capabilities.roomVersions = { "1", { { "1", "stable" } } }; } else { qCDebug(MAIN) << "Room versions:" << defaultRoomVersion() << "is default, full list:" << availableRoomVersions(); -- cgit v1.2.3 From 5937127b73a82fc86f6546397373ce9dbaf4e560 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 19 Nov 2019 17:57:41 +0900 Subject: BaseJob: Don't send accessToken if not needed; send again on 401 The first part closes #358; the second part is a workaround for non-standard cases when endpoints without security by the spec turn out to be secured (in particular, the case of authenticating media servers). --- lib/connection.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 5ee7e84e..c830557c 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -343,7 +343,8 @@ void Connection::logout() } const auto* job = callApi(); connect(job, &LogoutJob::finished, this, [this, job, syncWasRunning] { - if (job->status().good() || job->error() == BaseJob::ContentAccessError) { + if (job->status().good() || job->error() == BaseJob::Unauthorised + || job->error() == BaseJob::ContentAccessError) { if (d->syncLoopConnection) disconnect(d->syncLoopConnection); d->data->setToken({}); @@ -380,9 +381,9 @@ void Connection::sync(int timeout) }); connect(job, &SyncJob::failure, this, [this, job] { d->syncJob = nullptr; - if (job->error() == BaseJob::ContentAccessError) { - qCWarning(SYNCJOB) << "Sync job failed with ContentAccessError - " - "login expired?"; + if (job->error() == BaseJob::Unauthorised) { + qCWarning(SYNCJOB) + << "Sync job failed with Unauthorised - login expired?"; emit loginError(job->errorString(), job->rawDataSample()); } else emit syncError(job->errorString(), job->rawDataSample()); -- cgit v1.2.3 From d438c08e11d17dc9c005806a6036e1aeb769c54b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 19:39:24 +0300 Subject: Connection::uploadFile/Content(): refactoring around QIODevice::open() No more "The file is already open" log messages. --- lib/connection.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index c830557c..999f4382 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -624,12 +624,17 @@ UploadContentJob* Connection::uploadContent(QIODevice* contentSource, const QString& filename, const QString& overrideContentType) const { + Q_ASSERT(contentSource != nullptr); auto contentType = overrideContentType; if (contentType.isEmpty()) { contentType = QMimeDatabase() .mimeTypeForFileNameAndData(filename, contentSource) .name(); - contentSource->open(QIODevice::ReadOnly); + if (!contentSource->open(QIODevice::ReadOnly)) { + qCWarning(MAIN) << "Couldn't open content source" << filename + << "for reading:" << contentSource->errorString(); + return nullptr; + } } return callApi(contentSource, filename, contentType); } @@ -638,11 +643,6 @@ UploadContentJob* Connection::uploadFile(const QString& fileName, const QString& overrideContentType) { auto sourceFile = new QFile(fileName); - if (!sourceFile->open(QIODevice::ReadOnly)) { - qCWarning(MAIN) << "Couldn't open" << sourceFile->fileName() - << "for reading"; - return nullptr; - } return uploadContent(sourceFile, QFileInfo(*sourceFile).fileName(), overrideContentType); } -- cgit v1.2.3 From f6505a541fc0a2e02f9c79496488121a3e46fb01 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 19:47:07 +0300 Subject: BaseJob: prepare() -> initiate() + refactoring around it * BaseJob::initiate() now calls ConnectionData::submit() without relying on Connection to do that * ConnectionData::submit() is now the only site where a job enters Pending state * No more shortcuts to BaseJob::sendRequest(), even retries are sent through the ConnectionData submission queue * Additional validation in BaseJob::initiate() that the request data device is actually open (because QtNetwork API officially requires that, even if you can get away passing a closed QBuffer to it) --- lib/connection.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index 999f4382..c13568f1 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -1414,8 +1414,7 @@ void Connection::setLazyLoading(bool newValue) void Connection::run(BaseJob* job, RunningPolicy runningPolicy) const { connect(job, &BaseJob::failure, this, &Connection::requestFailed); - job->prepare(d->data.get(), runningPolicy & BackgroundRequest); - d->data->submit(job); + job->initiate(d->data.get(), runningPolicy & BackgroundRequest); } void Connection::getTurnServers() -- cgit v1.2.3 From 92264831077874511341b2dabae255649f741f54 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 23 Dec 2019 21:48:16 +0300 Subject: Connection::forgetRoom: slightly simplify code --- lib/connection.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/connection.cpp') diff --git a/lib/connection.cpp b/lib/connection.cpp index c13568f1..5bddbb83 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -804,7 +804,7 @@ ForgetRoomJob* Connection::forgetRoom(const QString& id) if (!room) room = d->roomMap.value({ id, true }); if (room && room->joinState() != JoinState::Leave) { - auto leaveJob = room->leaveRoom(); + auto leaveJob = leaveRoom(room); connect(leaveJob, &BaseJob::result, this, [this, leaveJob, forgetJob, room] { if (leaveJob->error() == BaseJob::Success -- cgit v1.2.3