[Libreoffice-commits] online.git: common/Unit.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/LOOLWSD.cpp

[Ashod Nakashian (via logerrit)]

 common/Unit.cpp        |    4 ++--
 wsd/DocumentBroker.cpp |    4 ++--
 wsd/DocumentBroker.hpp |    4 ++--
 wsd/LOOLWSD.cpp        |    9 ++++++++-
 4 files changed, 14 insertions(+), 7 deletions(-)

New commits:
commit 18ab1ccaed1db2dc7c306166fbc0730f8fad9570
Author:     Ashod Nakashian <ashod.nakashian at collabora.co.uk>
AuthorDate: Sun Feb 23 13:24:07 2020 -0500
Commit:     Ashod Nakashian <ashnakash at gmail.com>
CommitDate: Sun Feb 23 21:48:07 2020 +0100

    wsd: cleanly shutdown when unittest times out
    
    Termination flag is a very harsh way of exiting.
    It works in most cases, but not when we have a
    modified document. What happens is the following:
    
    Unit-test flags for termination.
    During session cleanup we have to save the modified doc.
    Because save is in progress we don't 'disconnect' the view.
    This leaves the view in loaded state until saving is done.
    But because of the termination flag we don't wait for saving.
    DocBroker sends 'exit' to child to forcefully exit.
    This causes at least one assertion due to active LOKWindows (Sidebar).
    
    Instead of the above, we flag for graceful shutdown from unittests,
    and after we wait to cleanup all DocBrokers, we flag for termination.
    This way, we get clean shutdown and all assertions/validations
    pass, while we guarantee never to deadlock the unittests,
    in case we end up waiting forever for shutdown to complete.
    
    Change-Id: I7fc34137ea373e329795b1ed0090261c085e955a
    Reviewed-on: https://gerrit.libreoffice.org/c/online/+/89308
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/common/Unit.cpp b/common/Unit.cpp
index 82cd82342..b34466c39 100644
--- a/common/Unit.cpp
+++ b/common/Unit.cpp
@@ -203,10 +203,10 @@ void UnitBase::exitTest(TestResult result)
         return;
     }
 
-    LOG_INF("exitTest: " << (int)result << ". Flagging for termination.");
+    LOG_INF("exitTest: " << (int)result << ". Flagging to shutdown.");
     _setRetValue = true;
     _retValue = result == TestResult::Ok ? EX_OK : EX_SOFTWARE;
-    SigUtil::setTerminationFlag();
+    SigUtil::requestShutdown();
     SocketPoll::wakeupWorld();
 }
 
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 1eed16ea0..88c22a303 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1381,8 +1381,8 @@ size_t DocumentBroker::removeSession(const std::string& id)
                 << " sessions. IsReadOnly: " << session->isReadOnly()
                 << ", IsViewLoaded: " << session->isViewLoaded() << ", IsWaitDisconnected: "
                 << session->inWaitDisconnected() << ", MarkToDestroy: " << _markToDestroy
-                << ", LastEditableSession: " << lastEditableSession
-                << ", dontSaveIfUnmodified: " << dontSaveIfUnmodified);
+                << ", LastEditableSession: " << lastEditableSession << ", DontSaveIfUnmodified: "
+                << dontSaveIfUnmodified << ", IsPossiblyModified: " << isPossiblyModified());
 
         // If last editable, save and don't remove until after uploading to storage.
         if (!lastEditableSession || !autoSave(isPossiblyModified(), dontSaveIfUnmodified))
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 890e89e38..5e3bea9aa 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -113,7 +113,7 @@ public:
             // Request the child to exit
             if (isAlive())
             {
-                LOG_DBG("Stopping ChildProcess [" << _pid << "]");
+                LOG_DBG("Stopping ChildProcess [" << _pid << "] by sending 'exit' command.");
                 sendTextFrame("exit");
             }
 
@@ -126,7 +126,7 @@ public:
             LOG_ERR("Error while closing child process: " << ex.what());
         }
 
-        _pid = -1;
+        _pid = -1; // Detach from child.
     }
 
     /// Kill or abandon the child.
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 678b4ddc6..f9fc062e2 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -3518,9 +3518,10 @@ int LOOLWSD::innerMain()
         SigUtil::requestShutdown();
     }
 #endif
+
     // Don't stop the DocBroker, they will exit.
     constexpr size_t sleepMs = 500;
-    constexpr size_t count = (COMMAND_TIMEOUT_MS * 4) / sleepMs;
+    constexpr size_t count = (COMMAND_TIMEOUT_MS * 6) / sleepMs;
     for (size_t i = 0; i < count; ++i)
     {
         std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
@@ -3535,6 +3536,12 @@ int LOOLWSD::innerMain()
         std::this_thread::sleep_for(std::chrono::milliseconds(sleepMs));
     }
 
+    if (UnitWSD::isUnitTesting() && !SigUtil::getTerminationFlag())
+    {
+        LOG_INF("Setting TerminationFlag to avoid deadlocking unittest.");
+        SigUtil::setTerminationFlag();
+    }
+
     // Disable thread checking - we'll now cleanup lots of things if we can
     Socket::InhibitThreadChecks = true;
     SocketPoll::InhibitThreadChecks = true;