From 898f35f2982e86f95c7fe061aa5e697c771a0d47 Mon Sep 17 00:00:00 2001 From: Christian Mollekopf Date: Wed, 25 Jul 2018 17:21:22 +0200 Subject: Avoid the socket probing and move the shutdown logic into resourceaccess. The problem was (as excercised by the last test in resourcecontroltest), that in this scenario we would: * trigger a synchronization that starts the resource, and then goes into a loop trying to connecting (KAsync::wait -> singleshot timer) * trigger a shutdown that would probe for the socket, not find it, and thus do nothing. * exit the testfunction, which somehow stops qtimer processing, meaning we are stuck in KAsync::wait. For now this is fixed by simply not probing for the socket. --- common/resourceaccess.cpp | 5 +++ common/resourceaccess.h | 6 ++++ common/resourcecontrol.cpp | 49 ++++++++++--------------- tests/CMakeLists.txt | 1 + tests/resourcecontroltest.cpp | 83 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 30 deletions(-) create mode 100644 tests/resourcecontroltest.cpp diff --git a/common/resourceaccess.cpp b/common/resourceaccess.cpp index 2fa753d..033acc9 100644 --- a/common/resourceaccess.cpp +++ b/common/resourceaccess.cpp @@ -424,6 +424,11 @@ KAsync::Job ResourceAccess::sendSecret(const QString &secret) return sendCommand(Sink::Commands::SecretCommand, fbb); } +KAsync::Job ResourceAccess::shutdown() +{ + return sendCommand(Sink::Commands::ShutdownCommand); +} + void ResourceAccess::open() { if (d->socket && d->socket->isValid()) { diff --git a/common/resourceaccess.h b/common/resourceaccess.h index ea3329d..e791236 100644 --- a/common/resourceaccess.h +++ b/common/resourceaccess.h @@ -86,6 +86,11 @@ public: return KAsync::null(); } + virtual KAsync::Job shutdown() + { + return KAsync::null(); + } + int getResourceStatus() const { return mResourceStatus; @@ -128,6 +133,7 @@ public: sendInspectionCommand(int inspectionType,const QByteArray &inspectionId, const QByteArray &domainType, const QByteArray &entityId, const QByteArray &property, const QVariant &expecedValue) Q_DECL_OVERRIDE; KAsync::Job sendFlushCommand(int flushType, const QByteArray &flushId) Q_DECL_OVERRIDE; KAsync::Job sendSecret(const QString &secret) Q_DECL_OVERRIDE; + KAsync::Job shutdown() Q_DECL_OVERRIDE; /** * Tries to connect to server, and returns a connected socket on success. */ diff --git a/common/resourcecontrol.cpp b/common/resourcecontrol.cpp index 5e8e09a..ea660c5 100644 --- a/common/resourcecontrol.cpp +++ b/common/resourcecontrol.cpp @@ -37,38 +37,27 @@ KAsync::Job ResourceControl::shutdown(const QByteArray &identifier) SinkTrace() << "shutdown " << identifier; auto time = QSharedPointer::create(); time->start(); - return ResourceAccess::connectToServer(identifier) - .then>( - [identifier, time](const KAsync::Error &error, QSharedPointer socket) { - if (error) { - SinkTrace() << "Resource is already closed."; - // Resource isn't started, nothing to shutdown - return KAsync::null(); + + auto resourceAccess = ResourceAccessFactory::instance().getAccess(identifier, ResourceConfig::getResourceType(identifier)); + return resourceAccess->shutdown() + .addToContext(resourceAccess) + .then([resourceAccess, time](KAsync::Future &future) { + SinkTrace() << "Shutdown command complete, waiting for shutdown." << Log::TraceTime(time->elapsed()); + if (!resourceAccess->isReady()) { + future.setFinished(); + return; + } + auto guard = new QObject; + QObject::connect(resourceAccess.data(), &ResourceAccess::ready, guard, [&future, guard](bool ready) { + if (!ready) { + //Protect against callback getting called twice. + delete guard; + future.setFinished(); } - // We can't currently reuse the socket - socket->close(); - auto resourceAccess = ResourceAccessFactory::instance().getAccess(identifier, ResourceConfig::getResourceType(identifier)); - resourceAccess->open(); - return resourceAccess->sendCommand(Sink::Commands::ShutdownCommand) - .addToContext(resourceAccess) - .then([resourceAccess, time](KAsync::Future &future) { - SinkTrace() << "Shutdown command complete, waiting for shutdown." << Log::TraceTime(time->elapsed()); - if (!resourceAccess->isReady()) { - future.setFinished(); - return; - } - auto guard = new QObject; - QObject::connect(resourceAccess.data(), &ResourceAccess::ready, guard, [&future, guard](bool ready) { - if (!ready) { - //Protect against callback getting called twice. - delete guard; - future.setFinished(); - } - }); - }).then([time] { - SinkTrace() << "Shutdown complete." << Log::TraceTime(time->elapsed()); - }); }); + }).then([time] { + SinkTrace() << "Shutdown complete." << Log::TraceTime(time->elapsed()); + }); } KAsync::Job ResourceControl::start(const QByteArray &identifier) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2706d5b..3bbbe37 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -55,6 +55,7 @@ auto_tests ( notificationtest entitystoretest upgradetest + resourcecontroltest ) if (WIN32) diff --git a/tests/resourcecontroltest.cpp b/tests/resourcecontroltest.cpp new file mode 100644 index 0000000..e66f444 --- /dev/null +++ b/tests/resourcecontroltest.cpp @@ -0,0 +1,83 @@ +#include + +#include "dummyresource/resourcefactory.h" +#include "resourcecontrol.h" +#include "store.h" +#include "testutils.h" +#include "test.h" +#include "resourceconfig.h" + +/** + * Test starting and stopping of resources. + */ +class ResourceControlTest : public QObject +{ + Q_OBJECT + + KAsync::Job socketIsAvailable(const QByteArray &identifier) + { + return Sink::ResourceAccess::connectToServer(identifier) + .then>( + [&](const KAsync::Error &error, QSharedPointer socket) { + if (error) { + return KAsync::value(false); + } + socket->close(); + return KAsync::value(true); + }); + + } + + bool blockingSocketIsAvailable(const QByteArray &identifier) + { + auto job = socketIsAvailable(identifier); + auto future = job.exec(); + future.waitForFinished(); + return future.value(); + } + +private slots: + + void initTestCase() + { + Sink::Test::initTest(); + auto factory = Sink::ResourceFactory::load("sink.dummy"); + QVERIFY(factory); + ::DummyResource::removeFromDisk("sink.dummy.instance1"); + ResourceConfig::addResource("sink.dummy.instance1", "sink.dummy"); + ::DummyResource::removeFromDisk("sink.dummy.instance2"); + ResourceConfig::addResource("sink.dummy.instance2", "sink.dummy"); + } + + void testResourceStart() + { + VERIFYEXEC(Sink::ResourceControl::start("sink.dummy.instance1")); + QVERIFY(blockingSocketIsAvailable("sink.dummy.instance1")); + } + + void testResourceShutdown() + { + QVERIFY(!blockingSocketIsAvailable("sink.dummy.instance2")); + VERIFYEXEC(Sink::ResourceControl::start("sink.dummy.instance2")); + QVERIFY(blockingSocketIsAvailable("sink.dummy.instance2")); + VERIFYEXEC(Sink::ResourceControl::shutdown("sink.dummy.instance2")); + QVERIFY(!blockingSocketIsAvailable("sink.dummy.instance2")); + } + + //This will produce a race where the synchronize command starts the resource, + //the shutdown command doesn't shutdown because it doesn't realize that the resource is up, + //and the resource ends up getting started, but doing nothing. + void testResourceShutdownAfterStartByCommand() + { + QVERIFY(!blockingSocketIsAvailable("sink.dummy.instance2")); + auto future = Sink::Store::synchronize(Sink::SyncScope{}.resourceFilter("sink.dummy.instance2")).exec(); + + VERIFYEXEC(Sink::ResourceControl::shutdown("sink.dummy.instance2")); + + QVERIFY(!blockingSocketIsAvailable("sink.dummy.instance2")); + } + +}; + +QTEST_MAIN(ResourceControlTest) +#include "resourcecontroltest.moc" -- cgit v1.2.3