From 8ace3817ea8fc31a40b28634da2d73b590a5bafc Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 27 Nov 2024 16:55:31 +0100 Subject: [PATCH 1/5] Refresh doc comments for BaseJob::retryScheduled() [skip ci] --- Quotient/jobs/basejob.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Quotient/jobs/basejob.h b/Quotient/jobs/basejob.h index c31294b63..1fe0aa063 100644 --- a/Quotient/jobs/basejob.h +++ b/Quotient/jobs/basejob.h @@ -277,9 +277,9 @@ public Q_SLOTS: void statusChanged(Quotient::BaseJob::Status newStatus); //! \brief A retry of the network request is scheduled after the previous request failed - //! \param nextAttempt the 1-based number of attempt (will always be more than 1) + //! \param nextRetryNumber the number of the next retry, starting from 1 //! \param inMilliseconds the interval after which the next attempt will be taken - void retryScheduled(int nextAttempt, Quotient::BaseJob::duration_ms_t inMilliseconds); + void retryScheduled(int nextRetryNumber, Quotient::BaseJob::duration_ms_t inMilliseconds); //! \brief The job has been rate-limited //! From c1ef9310b30b1fefa789756cdaafb12d6cbcb187 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 27 Nov 2024 17:00:18 +0100 Subject: [PATCH 2/5] Use std::unique_ptr instead of QScopedPointer QScopedPointer is getting partially deprecated since Qt 6.1, and there are few reasons to use it instead of std::unique_ptr these days. --- Quotient/jobs/downloadfilejob.cpp | 15 ++++++++------- Quotient/util.h | 12 +++++++++++- quotest/quotest.cpp | 9 +++++---- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/Quotient/jobs/downloadfilejob.cpp b/Quotient/jobs/downloadfilejob.cpp index 28079eb00..718604b07 100644 --- a/Quotient/jobs/downloadfilejob.cpp +++ b/Quotient/jobs/downloadfilejob.cpp @@ -23,15 +23,16 @@ class Q_DECL_HIDDEN DownloadFileJob::Private { explicit Private(QString serverName, QString mediaId, const QString& localFilename) : serverName(std::move(serverName)) , mediaId(std::move(mediaId)) - , targetFile(!localFilename.isEmpty() ? new QFile(localFilename) : nullptr) - , tempFile(!localFilename.isEmpty() ? new QFile(targetFile->fileName() + ".qtntdownload"_L1) - : new QTemporaryFile()) + , targetFile(!localFilename.isEmpty() ? std::make_unique(localFilename) : nullptr) + , tempFile(!localFilename.isEmpty() + ? std::make_unique(targetFile->fileName() + ".qtntdownload"_L1) + : std::make_unique()) {} QString serverName; QString mediaId; - QScopedPointer targetFile; - QScopedPointer tempFile; + std::unique_ptr targetFile; + std::unique_ptr tempFile; std::optional encryptedFileMetadata; }; @@ -156,8 +157,8 @@ BaseJob::Status DownloadFileJob::prepareResult() } } else { if (d->encryptedFileMetadata.has_value()) { - QScopedPointer tempTempFile(new QTemporaryFile); - if (!tempTempFile->open(QFile::ReadWrite)) { + std::unique_ptr tempTempFile = std::make_unique(); + if (!tempTempFile->isWritable()) { qCWarning(JOBS) << "Failed to open temporary file for decryption" << tempTempFile->errorString(); return { FileError, "Couldn't open temporary file for decryption"_L1 }; diff --git a/Quotient/util.h b/Quotient/util.h index 1ced363ca..dde11c1ce 100644 --- a/Quotient/util.h +++ b/Quotient/util.h @@ -8,10 +8,11 @@ #include #include +#include #include #include +#include #include -#include #include #include @@ -230,6 +231,15 @@ inline std::pair findFirstOf(InputIt first, InputIt last, return { last, sLast }; } +//! \brief Common custom deleter for std::unique_ptr and QScopedPointer +//! +//! Since Qt 6, this is merely an alias for QScopedPointerDeleteLater (which is suitable +//! for std::unique_ptr too). +using DeleteLater = QScopedPointerDeleteLater; + +template T> +using QObjectHolder = std::unique_ptr; + //! \brief An owning implementation pointer //! //! This is basically std::unique_ptr<> to hold your pimpl's but without having diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp index 737fc4c73..1af53adf2 100644 --- a/quotest/quotest.cpp +++ b/quotest/quotest.cpp @@ -470,12 +470,13 @@ TEST_IMPL(sendFile) return false; } -void getResource(const QUrl& url, QScopedPointer& r, - QEventLoop& el) +using NetworkReplyPtr = QObjectHolder; + +void getResource(const QUrl& url, NetworkReplyPtr& r, QEventLoop& el) { r.reset(NetworkAccessManager::instance()->get(QNetworkRequest(url))); QObject::connect( - r.data(), &QNetworkReply::finished, &el, + r.get(), &QNetworkReply::finished, &el, [url, &r, &el] { if (r->error() != QNetworkReply::NoError) getResource(url, r, el); @@ -490,7 +491,7 @@ bool testDownload(const QUrl& url) // The actual test is separate from the download invocation to help debugging const auto results = QtConcurrent::blockingMapped(QVector{ 1, 2, 3 }, [url](int) { thread_local QEventLoop el; - thread_local QScopedPointer reply{}; + thread_local NetworkReplyPtr reply{}; getResource(url, reply, el); el.exec(); return reply->error(); From e9e2dcd1f896f2eb84f794a440574809ea35ac53 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 27 Nov 2024 17:01:57 +0100 Subject: [PATCH 3/5] Make autotests even more robust against timeouts These now use the information about job back-off strategies to calculate precisely how long they should wait for a job to complete. --- autotests/testolmaccount.cpp | 46 +++++++++++++++++++----------------- autotests/testutils.h | 13 +++++++--- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/autotests/testolmaccount.cpp b/autotests/testolmaccount.cpp index eb0214c26..ef6296f39 100644 --- a/autotests/testolmaccount.cpp +++ b/autotests/testolmaccount.cpp @@ -214,7 +214,7 @@ void TestOlmAccount::uploadOneTimeKeys() QCOMPARE(oneTimeKeyCounts.value(Curve25519Key), 5); }, [] { QFAIL("upload failed"); }); - QVERIFY(waitForFuture(uploadFuture)); + QVERIFY(waitForJob(uploadFuture)); } void TestOlmAccount::uploadSignedOneTimeKeys() @@ -233,7 +233,7 @@ void TestOlmAccount::uploadSignedOneTimeKeys() QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), nKeys); }, [] { QFAIL("upload failed"); }); - QVERIFY(waitForFuture(uploadFuture)); + QVERIFY(waitForJob(uploadFuture)); } void TestOlmAccount::uploadKeys() @@ -249,7 +249,7 @@ void TestOlmAccount::uploadKeys() QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 1); }, [] { QFAIL("upload failed"); }); - QVERIFY(waitForFuture(request)); + QVERIFY(waitForJob(request)); } void TestOlmAccount::queryTest() @@ -265,7 +265,7 @@ void TestOlmAccount::queryTest() .then([](const QHash& aliceOneTimeKeyCounts) { QCOMPARE(aliceOneTimeKeyCounts.value(SignedCurve25519Key), 1); }); - QVERIFY(waitForFuture(aliceUploadKeysRequest)); + QVERIFY(waitForJob(aliceUploadKeysRequest)); auto bobOlm = bob->olmAccount(); bobOlm->generateOneTimeKeys(1); @@ -274,7 +274,7 @@ void TestOlmAccount::queryTest() .then([](const QHash& bobOneTimeKeyCounts) { QCOMPARE(bobOneTimeKeyCounts.value(SignedCurve25519Key), 1); }); - QVERIFY(waitForFuture(bobUploadKeysRequest)); + QVERIFY(waitForJob(bobUploadKeysRequest)); // Each user is requests each other's keys. const QHash deviceKeysForBob{ { bob->userId(), {} } }; @@ -292,7 +292,7 @@ void TestOlmAccount::queryTest() QCOMPARE(aliceDevKeys.keys, bobOlm->deviceKeys().keys); QCOMPARE(aliceDevKeys.signatures, bobOlm->deviceKeys().signatures); }); - QVERIFY(waitForFuture(queryBobKeysResult)); + QVERIFY(waitForJob(queryBobKeysResult)); const QHash deviceKeysForAlice{ { alice->userId(), {} } }; const auto queryAliceKeysResult = @@ -309,7 +309,7 @@ void TestOlmAccount::queryTest() QCOMPARE(devKeys.keys, aliceOlm->deviceKeys().keys); QCOMPARE(devKeys.signatures, aliceOlm->deviceKeys().signatures); }); - QVERIFY(waitForFuture(queryAliceKeysResult)); + QVERIFY(waitForJob(queryAliceKeysResult)); } void TestOlmAccount::claimKeys() @@ -324,7 +324,7 @@ void TestOlmAccount::claimKeys() .then([bob](const QHash& oneTimeKeyCounts) { QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 1); }); - QVERIFY(waitForFuture(request)); + QVERIFY(waitForJob(request)); // Alice retrieves bob's keys & claims one signed one-time key. const QHash deviceKeysToQuery{ { bob->userId(), {} } }; @@ -336,7 +336,7 @@ void TestOlmAccount::claimKeys() QVERIFY(verifyIdentitySignature(bobDevices.value(bob->deviceId()), bob->deviceId(), bob->userId())); }); - QVERIFY(waitForFuture(queryKeysJob)); + QVERIFY(waitForJob(queryKeysJob)); // Retrieve the identity key for the current device to check after claiming // const auto& bobEd25519 = @@ -363,7 +363,7 @@ void TestOlmAccount::claimKeys() } }, [] { QFAIL("Claim job failed"); }); - QVERIFY(waitForFuture(claimKeysJob)); + QVERIFY(waitForJob(claimKeysJob)); } void TestOlmAccount::claimMultipleKeys() @@ -380,7 +380,7 @@ void TestOlmAccount::claimMultipleKeys() .then([](const QHash& oneTimeKeyCounts) { QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10); }); - QVERIFY(waitForFuture(aliceUploadKeyRequest)); + QVERIFY(waitForJob(aliceUploadKeyRequest)); auto olm1 = alice1->olmAccount(); olm1->generateOneTimeKeys(10); @@ -389,7 +389,7 @@ void TestOlmAccount::claimMultipleKeys() .then([](const QHash& oneTimeKeyCounts) { QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10); }); - QVERIFY(waitForFuture(alice1UploadKeyRequest)); + QVERIFY(waitForJob(alice1UploadKeyRequest)); auto olm2 = alice2->olmAccount(); olm2->generateOneTimeKeys(10); @@ -398,7 +398,7 @@ void TestOlmAccount::claimMultipleKeys() .then([](const QHash& oneTimeKeyCounts) { QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10); }); - QVERIFY(waitForFuture(alice2UploadKeyRequest)); + QVERIFY(waitForJob(alice2UploadKeyRequest)); // Bob will claim keys from all Alice's devices CREATE_CONNECTION(bob, "bob3"_L1, "secret"_L1, "BobPhone"_L1) @@ -412,22 +412,24 @@ void TestOlmAccount::claimMultipleKeys() bob->callApi(oneTimeKeys).then([alice](const ClaimKeysJob::Response& r) { QCOMPARE(r.oneTimeKeys.value(alice->userId()).size(), 3); }); - QVERIFY(waitForFuture(claimResult)); + QVERIFY(waitForJob(claimResult)); } void TestOlmAccount::enableEncryption() { CREATE_CONNECTION(alice, "alice9"_L1, "secret"_L1, "AlicePhone"_L1) - const auto futureRoom = alice->createRoom(Connection::PublishRoom, {}, {}, {}, {}) - .then([alice](const QString& roomId) { - auto room = alice->room(roomId); - room->activateEncryption(); - return room; - }); - QVERIFY(waitForFuture(futureRoom)); + auto createRoomJob = alice->createRoom(Connection::PublishRoom, {}, {}, {}, {}); + auto futureEncryptedRoom = createRoomJob.then([alice](const QString& roomId) { + auto room = alice->room(roomId); + room->activateEncryption(); + return room; + }); + QVERIFY(waitForJob(createRoomJob)); alice->syncLoop(); - QVERIFY(QTest::qWaitFor([room = futureRoom.result()] { return room->usesEncryption(); }, 40000)); + // Give extra 5 seconds for the network roundtrip + QVERIFY(QTest::qWaitFor(std::bind_front(&Room::usesEncryption, futureEncryptedRoom.result()), + SyncJob::defaultTimeoutMillis * 2 + 5000)); } QTEST_GUILESS_MAIN(TestOlmAccount) diff --git a/autotests/testutils.h b/autotests/testutils.h index 77291c1b7..bd4d78a0e 100644 --- a/autotests/testutils.h +++ b/autotests/testutils.h @@ -11,6 +11,8 @@ namespace Quotient { class Connection; +template +class JobHandle; std::shared_ptr createTestConnection(QLatin1StringView localUserName, QLatin1StringView secret, @@ -22,8 +24,13 @@ std::shared_ptr createTestConnection(QLatin1StringView localUserName if (!VAR) \ QFAIL("Could not set up test connection"); -inline bool waitForFuture(const auto& ft) - requires requires { ft.isFinished(); } +template +inline bool waitForJob(const Quotient::JobHandle& job) { - return QTest::qWaitFor([ft] { return ft.isFinished(); }, 40000); + const auto& [timeouts, retryIntervals, _] = job->currentBackoffStrategy(); + return QTest::qWaitFor([job] { return job.isFinished(); }, + std::chrono::milliseconds( + std::reduce(timeouts.cbegin(), timeouts.cend()) + + std::reduce(retryIntervals.cbegin(), retryIntervals.cend())) + .count()); } From fb51cc121cf306416fd7470f0efc8a7d463dc97f Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 28 Nov 2024 22:12:45 +0100 Subject: [PATCH 4/5] findIndirect(): factor out common constraint into a concept --- Quotient/ranges_extras.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/Quotient/ranges_extras.h b/Quotient/ranges_extras.h index 38ee933a1..9555409ba 100644 --- a/Quotient/ranges_extras.h +++ b/Quotient/ranges_extras.h @@ -6,8 +6,13 @@ namespace Quotient { //! Same as std::projected but Proj is checked against the reference under the iterator template > Proj> -using IndirectlyProjected = std::projected, Proj>; + std::indirectly_regular_unary_invocable> ProjT> +using IndirectlyProjected = std::projected, ProjT>; + +//! Same as std::indirectly_comparable but uses IndirectlyProjected<> instead of std::projected<> +template +concept IndirectlyProjectedComparable = + std::indirect_binary_predicate, const ValT*>; //! \brief Find a value in a container of (smart) pointers //! @@ -17,12 +22,9 @@ using IndirectlyProjected = std::projected, Proj>; //! searching for events that match a specific simple criterion; e.g., to find an event with a given //! id in a container you can now write `findIndirect(events, eventId, &RoomEvent::id);` instead //! of having to supply your own lambda to dereference the timeline item and check the event id. -template - requires std::indirect_binary_predicate, const ValT*> -// Most of constraints here (including IndirectlyProjected) are based on the definition of -// std::ranges::find and things around it -inline constexpr auto findIndirect(IterT from, IterT to, const ValT& value, Proj proj = {}) +template + requires IndirectlyProjectedComparable +inline constexpr auto findIndirect(IterT from, IterT to, const ValT& value, ProjT proj = {}) { return std::ranges::find(from, to, value, [p = std::move(proj)](auto& itemPtr) { return std::invoke(p, *itemPtr); @@ -30,11 +32,9 @@ inline constexpr auto findIndirect(IterT from, IterT to, const ValT& value, Proj } //! The overload of findIndirect for ranges -template - requires std::indirect_binary_predicate< - std::ranges::equal_to, IndirectlyProjected, Proj>, - const ValT*> -inline constexpr auto findIndirect(RangeT&& range, const ValT& value, Proj proj = {}) +template + requires IndirectlyProjectedComparable, ValT, ProjT> +inline constexpr auto findIndirect(RangeT&& range, const ValT& value, ProjT proj = {}) { return findIndirect(std::ranges::begin(range), std::ranges::end(range), value, std::move(proj)); } From fe3fcc89de0c36a9ae32203297e16cb9605415fe Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 28 Nov 2024 22:13:21 +0100 Subject: [PATCH 5/5] ranges_extras.h: findIndex() Upstreaming of a piece from Quaternion. --- Quotient/ranges_extras.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Quotient/ranges_extras.h b/Quotient/ranges_extras.h index 9555409ba..20a68c947 100644 --- a/Quotient/ranges_extras.h +++ b/Quotient/ranges_extras.h @@ -39,4 +39,16 @@ inline constexpr auto findIndirect(RangeT&& range, const ValT& value, ProjT proj return findIndirect(std::ranges::begin(range), std::ranges::end(range), value, std::move(proj)); } +//! \brief An indexOf() alternative for any range +//! +//! Unlike QList::indexOf(), returns `range.size()` if \p value is not found +template + requires std::indirectly_comparable, const ValT*, + std::ranges::equal_to, ProjT> +inline auto findIndex(const RangeT& range, const ValT& value, ProjT proj = {}) +{ + using namespace std::ranges; + return distance(begin(range), find(range, value, std::move(proj))); +} + }