Polish libsyncthing

* Adapt tests
* Remove duplicated code
* Stop Syncthing from a different thread since
  it blocks until Syncthing has stopped

libsyncthing/go/src/github.com/syncthing/syncthing

@@ -1 +1 @@
-Subproject commit 51440c77d846bbca1de48d58d6fc5ad69758ca7a
+Subproject commit afc11f78b983b6b6053776165e526b24cc945e99

libsyncthing/interface.cpp

@@ -132,9 +132,9 @@ void setLoggingCallback(LoggingCallback &&callback)
  * \remark
  * - Does nothing if Syncthing is already running.
  * - Blocks the current thread as long as the instance is running.
- *   Use eg. std::thread(runSyncthing, options) to run it in another thread.
+ *   Use e.g. std::thread(runSyncthing, options) to run it in another thread.
  */
-long long runSyncthing(const RuntimeOptions &options)
+std::int64_t runSyncthing(const RuntimeOptions &options)
 {
     const RunningState runningState;
     if (!runningState) {
@@ -155,16 +155,15 @@ bool isSyncthingRunning()
 
 /*!
  * \brief Stops Syncthing if it is running; otherwise does nothing.
- * \returns Might be called from any thread.
+ * \returns Returns the Syncthing exit code (as usual, zero means no error).
- * \todo Make this actually work. Currently crashes happen after stopping Syncthing.
+ * \remarks
- * \sa https://github.com/syncthing/syncthing/issues/4085
+ * - Might be called from any thread.
+ * - Blocks the current thread until the instance has been stopped.
+ *   Use e.g. std::thread(stopSyncthing) to run it in another thread.
  */
-void stopSyncthing()
+std::int64_t stopSyncthing()
 {
-    if (!syncthingRunning.load()) {
+    return ::libst_stop_syncthing();
-        return;
-    }
-    ::libst_stop_syncthing();
 }
 
 /*!

libsyncthing/interface.h

@@ -3,6 +3,7 @@
 
 #include "./global.h"
 
+#include <cstdint>
 #include <functional>
 #include <string>
 #include <vector>
@@ -30,9 +31,9 @@ using LoggingCallback = std::function<void(LogLevel, const char *message, std::s
 
 void LIB_SYNCTHING_EXPORT setLoggingCallback(const LoggingCallback &callback);
 void LIB_SYNCTHING_EXPORT setLoggingCallback(LoggingCallback &&callback);
-long long LIB_SYNCTHING_EXPORT runSyncthing(const RuntimeOptions &options = RuntimeOptions{});
+std::int64_t LIB_SYNCTHING_EXPORT runSyncthing(const RuntimeOptions &options = RuntimeOptions{});
 bool LIB_SYNCTHING_EXPORT isSyncthingRunning();
-void LIB_SYNCTHING_EXPORT stopSyncthing();
+std::int64_t LIB_SYNCTHING_EXPORT stopSyncthing();
 std::string LIB_SYNCTHING_EXPORT ownDeviceId();
 std::string LIB_SYNCTHING_EXPORT syncthingVersion();
 std::string LIB_SYNCTHING_EXPORT longSyncthingVersion();

libsyncthing/tests/interfacetests.cpp

@@ -13,6 +13,7 @@
 #include <cstdlib>
 #include <filesystem>
 #include <functional>
+#include <thread>
 
 #include <unistd.h>
 
@@ -27,16 +28,17 @@ using namespace CPPUNIT_NS;
  */
 class InterfaceTests : public TestFixture {
     CPPUNIT_TEST_SUITE(InterfaceTests);
-    CPPUNIT_TEST(testRunWidthConfig);
+    CPPUNIT_TEST(testInitialState);
     CPPUNIT_TEST(testVersion);
+    CPPUNIT_TEST(testRunWidthConfig);
     CPPUNIT_TEST_SUITE_END();
 
 public:
     InterfaceTests();
 
     void testInitialState();
-    void testRunWidthConfig();
     void testVersion();
+    void testRunWidthConfig();
 
     void setUp();
     void tearDown();
@@ -82,12 +84,12 @@ string InterfaceTests::setupConfigDir()
             if (!dir.is_directory() || dirPath == "." || dirPath == "..") {
                 continue;
             }
-            const auto subdirIterator = filesystem::directory_iterator(configDir % '/' + dirPath);
+            const auto subdirIterator = filesystem::directory_iterator(dirPath);
             for (const auto &file : subdirIterator) {
                 if (file.is_directory()) {
                     continue;
                 }
-                const auto toRemove = configDir % '/' % dirPath % '/' + file.path();
+                const auto toRemove = file.path().string();
                 CPPUNIT_ASSERT_EQUAL_MESSAGE("removing " + toRemove, 0, remove(toRemove.data()));
             }
         }
@@ -110,7 +112,20 @@ void InterfaceTests::testInitialState()
 }
 
 /*!
- * \brief Tests running Syncthing.
+ * \brief Tests whether the version() functions at least return something.
+ */
+void InterfaceTests::testVersion()
+{
+    const auto version(syncthingVersion());
+    const auto longVersion(longSyncthingVersion());
+    cout << "\nversion: " << version;
+    cout << "\nlong version: " << longVersion << endl;
+    CPPUNIT_ASSERT(!version.empty());
+    CPPUNIT_ASSERT(!longVersion.empty());
+}
+
+/*!
+ * \brief Test helper for running Syncthing and checking log according to test configuration.
  */
 void InterfaceTests::testRun(const std::function<long long()> &runFunction)
 {
@@ -135,32 +150,54 @@ void InterfaceTests::testRun(const std::function<long long()> &runFunction)
             myIdAnnounced = true;
         } else if (startsWith(msg, "Single thread SHA256 performance is")) {
             performanceAnnounced = true;
-        } else if (msg == "Ready to synchronize test1 (readwrite)") {
+        } else if (msg == "Ready to synchronize test1 (sendreceive)") {
             testDir1Ready = true;
-        } else if (msg == "Ready to synchronize test2 (readwrite)") {
+        } else if (msg == "Ready to synchronize test2 (sendreceive)") {
             testDir2Ready = true;
         } else if (msg == "Device 6EIS2PN-J2IHWGS-AXS3YUL-HC5FT3K-77ZXTLL-AKQLJ4C-7SWVPUS-AZW4RQ4 is \"Test dev 1\" at [dynamic]") {
             testDev1Ready = true;
         } else if (msg == "Device MMGUI6U-WUEZQCP-XZZ6VYB-LCT4TVC-ER2HAVX-QYT6X7D-S6ZSG2B-323KLQ7 is \"Test dev 2\" at [tcp://192.168.2.2:22001]") {
             testDev2Ready = true;
-        } else if (msg == "Shutting down") {
+        } else if (msg == "Exiting") {
             shutDownLogged = true;
         }
 
         // print the message on cout (which results in duplicated messages, but allows to check whether we've got everything)
-        cout << "logging callback (" << static_cast<std::underlying_type<LogLevel>::type>(logLevel) << ": ";
+        cout << "logging callback (" << static_cast<std::underlying_type<LogLevel>::type>(logLevel) << "): ";
         cout.write(message, static_cast<std::streamsize>(messageSize));
         cout << endl;
 
         // stop Syncthing again if the found the messages we've been looking for or we've timed out
         const auto timeout((DateTime::gmtNow() - startTime) > TimeSpan::fromSeconds(30));
         if (!timeout && (!myIdAnnounced || !performanceAnnounced || !testDir1Ready || !testDev1Ready || !testDev2Ready)) {
+            // log status
+            cout << "still wating for:";
+            if (!myIdAnnounced) {
+                cout << " myIdAnnounced";
+            }
+            if (!performanceAnnounced) {
+                cout << " performanceAnnounced";
+            }
+            if (!testDir1Ready) {
+                cout << " testDir1Ready";
+            }
+            if (!testDir2Ready) {
+                cout << " testDir2Ready";
+            }
+            if (!testDev1Ready) {
+                cout << " testDev1Ready";
+            }
+            if (!testDev2Ready) {
+                cout << " testDev2Ready";
+            }
+            cout << endl;
             return;
         }
         if (!shuttingDown) {
             cerr << "stopping Syncthing again" << endl;
             shuttingDown = true;
-            stopSyncthing();
+            std::thread stopThread(stopSyncthing);
+            stopThread.detach();
         }
     });
 
@@ -180,22 +217,12 @@ void InterfaceTests::testRun(const std::function<long long()> &runFunction)
     sleep(5);
 }
 
+/*!
+ * \brief Tests whether Syncthing can be started (and stopped again) using the specified test config.
+ */
 void InterfaceTests::testRunWidthConfig()
 {
     RuntimeOptions options;
     options.configDir = setupConfigDir();
-    testRun(bind(static_cast<long long (*)(const RuntimeOptions &)>(&runSyncthing), cref(options)));
+    testRun(bind(static_cast<std::int64_t (*)(const RuntimeOptions &)>(&runSyncthing), cref(options)));
-}
-
-/*!
- * \brief Tests whether the version() functions at least return something.
- */
-void InterfaceTests::testVersion()
-{
-    const auto version(syncthingVersion());
-    const auto longVersion(longSyncthingVersion());
-    cout << "\nversion: " << version;
-    cout << "\nlong version: " << longVersion << endl;
-    CPPUNIT_ASSERT(!version.empty());
-    CPPUNIT_ASSERT(!longVersion.empty());
 }

widgets/misc/syncthinglauncher.cpp

@@ -53,9 +53,11 @@ bool SyncthingLauncher::isLibSyncthingAvailable()
 
 /*!
  * \brief Launches a Syncthing instance using the specified \a arguments.
- * \remarks
+ *
- * - Does nothing if already running an instance.
+ * To use the internal library, leave \a program empty. In this case \a arguments are ignored.
- * - To use the internal library, leave \a program empty. Otherwise it must be the path the external Syncthing executable.
+ * Otherwise \a program must be the path the external Syncthing executable.
+ *
+ * \remarks Does nothing if already running an instance.
  */
 void SyncthingLauncher::launch(const QString &program, const QStringList &arguments)
 {
@@ -71,7 +73,7 @@ void SyncthingLauncher::launch(const QString &program, const QStringList &argume
     }
 
     // use libsyncthing
-    m_future = QtConcurrent::run(this, static_cast<void (SyncthingLauncher::*)()>(&SyncthingLauncher::runLibSyncthing));
+    m_future = QtConcurrent::run(this, &SyncthingLauncher::runLibSyncthing, LibSyncthing::RuntimeOptions{});
 }
 
 /*!
@@ -106,8 +108,7 @@ void SyncthingLauncher::launch(const LibSyncthing::RuntimeOptions &runtimeOption
         return;
     }
     m_manuallyStopped = false;
-    m_future = QtConcurrent::run(
+    m_future = QtConcurrent::run(this, &SyncthingLauncher::runLibSyncthing, runtimeOptions);
-        this, static_cast<void (SyncthingLauncher::*)(const LibSyncthing::RuntimeOptions &)>(&SyncthingLauncher::runLibSyncthing), runtimeOptions);
 }
 
 void SyncthingLauncher::terminate()
@@ -117,9 +118,7 @@ void SyncthingLauncher::terminate()
         m_process.stopSyncthing();
     } else if (m_future.isRunning()) {
         m_manuallyStopped = true;
-#ifdef SYNCTHINGWIDGETS_USE_LIBSYNCTHING
+        QtConcurrent::run(this, &SyncthingLauncher::stopLibSyncthing);
-        LibSyncthing::stopSyncthing();
-#endif
     }
 }
 
@@ -130,10 +129,7 @@ void SyncthingLauncher::kill()
         m_process.killSyncthing();
     } else if (m_future.isRunning()) {
         m_manuallyStopped = true;
-#ifdef SYNCTHINGWIDGETS_USE_LIBSYNCTHING
+        QtConcurrent::run(this, &SyncthingLauncher::stopLibSyncthing);
-        // FIXME: any chance to try harder?
-        LibSyncthing::stopSyncthing();
-#endif
     }
 }
 
@@ -206,17 +202,11 @@ void SyncthingLauncher::runLibSyncthing(const LibSyncthing::RuntimeOptions &runt
 #endif
 }
 
-void SyncthingLauncher::runLibSyncthing()
+void SyncthingLauncher::stopLibSyncthing()
 {
 #ifdef SYNCTHINGWIDGETS_USE_LIBSYNCTHING
-    LibSyncthing::setLoggingCallback(bind(&SyncthingLauncher::handleLoggingCallback, this, _1, _2, _3));
+    LibSyncthing::stopSyncthing();
-    emit runningChanged(true);
+    // no need to emit exited/runningChanged here; that is already done in runLibSyncthing()
-    const auto exitCode = LibSyncthing::runSyncthing();
-    emit exited(static_cast<int>(exitCode), exitCode == 0 ? QProcess::NormalExit : QProcess::CrashExit);
-    emit runningChanged(false);
-#else
-    emit outputAvailable("libsyncthing support not enabled");
-    emit exited(-1, QProcess::CrashExit);
 #endif
 }
 

widgets/misc/syncthinglauncher.h

@@ -55,7 +55,7 @@ private Q_SLOTS:
     void handleProcessFinished(int exitCode, QProcess::ExitStatus exitStatus);
     void handleLoggingCallback(LibSyncthing::LogLevel, const char *message, std::size_t messageSize);
     void runLibSyncthing(const LibSyncthing::RuntimeOptions &runtimeOptions);
-    void runLibSyncthing();
+    void stopLibSyncthing();
 
 private:
     SyncthingProcess m_process;