VCS: Fix time-out handling for synchronous processes.

Introduce static utilities to Utils::SynchronousProcess
for synchronous processes that mimicks the handling
of Utils::SynchronousProcess (apply timeout after no
more data are available on stdout/stderr as opposed
to waitForFinished()).

Task-number: QTCREATORBUG-777

src/libs/utils/synchronousprocess.cpp

@@ -288,7 +288,7 @@ void SynchronousProcess::slotTimeout()
     if (++m_d->m_hangTimerCount > m_d->m_maxHangTimerCount) {
         if (debug)
             qDebug() << Q_FUNC_INFO << "HANG detected, killing";
-        m_d->m_process.kill();
+        SynchronousProcess::stopProcess(m_d->m_process);
         m_d->m_result.result = SynchronousProcessResponse::Hang;
     } else {
         if (debug)
@@ -399,6 +399,43 @@ void SynchronousProcess::processStdErr(bool emitSignals)
     }
 }
 
+// Static utilities: Keep running as long as it gets data.
+bool SynchronousProcess::readDataFromProcess(QProcess &p, int timeOutMS,
+                                             QByteArray *stdOut, QByteArray *stdErr)
+{
+    if (p.state() != QProcess::Running) {
+        qWarning("readDataFromProcess: Process in non-running state passed in.");
+        return false;
+    }
+
+    // Keep the process running until it has no longer has data
+    bool finished = false;
+    bool hasData = false;
+    do {
+        finished = p.waitForFinished(timeOutMS);
+        if ( (hasData = p.bytesAvailable()) ) {
+            const QByteArray newStdOut = p.readAllStandardOutput();
+            const QByteArray newStdErr = p.readAllStandardError();
+            if (stdOut)
+                stdOut->append(newStdOut);
+            if (stdErr)
+                stdErr->append(newStdErr);
+        }
+    } while (hasData && !finished);
+    return finished;
+}
+
+bool SynchronousProcess::stopProcess(QProcess &p)
+{
+    if (p.state() != QProcess::Running)
+        return true;
+    p.terminate();
+    if (p.waitForFinished(300))
+        return true;
+    p.kill();
+    return p.waitForFinished(300);
+}
+
 // Path utilities
 
 enum  OS_Type { OS_Mac, OS_Windows, OS_Unix };

src/libs/utils/synchronousprocess.h

@@ -82,7 +82,12 @@ QTCREATOR_UTILS_EXPORT QDebug operator<<(QDebug str, const SynchronousProcessRes
  * The stdOutBuffered(), stdErrBuffered() signals are emitted with complete
  * lines based on the '\n' marker if they are enabled using
  * stdOutBufferedSignalsEnabled()/setStdErrBufferedSignalsEnabled().
- * They would typically be used for log windows. */
+ * They would typically be used for log windows.
+ *
+ * There is a timeout handling that takes effect after the last data have been
+ * read from stdout/stdin (as opposed to waitForFinished(), which measures time
+ * since it was invoked). It is thus also suitable for slow processes that continously
+ * output data (like version system operations).  */
 
 class QTCREATOR_UTILS_EXPORT SynchronousProcess : public QObject
 {
@@ -91,7 +96,8 @@ public:
     SynchronousProcess();
     virtual ~SynchronousProcess();
 
-    /* Timeout for hanging processes (no reaction on stderr/stdout)*/
+    /* Timeout for hanging processes (triggers after no more output
+     * occurs on stderr/stdout). */
     void setTimeout(int timeoutMS);
     int timeout() const;
 
@@ -118,6 +124,16 @@ public:
 
     SynchronousProcessResponse run(const QString &binary, const QStringList &args);
 
+    // Static helper for running a process synchronously in the foreground with timeout
+    // detection similar SynchronousProcess' handling (taking effect after no more output
+    // occurs on stderr/stdout as opposed to waitForFinished()). Returns false if a timeout
+    // occurs. Checking of the process' exit state/code still has to be done.
+    static bool readDataFromProcess(QProcess &p, int timeOutMS,
+                                    QByteArray *stdOut = 0, QByteArray *stdErr = 0);
+    // Stop a process by first calling terminate() (allowing for signal handling) and
+    // then kill().
+    static bool stopProcess(QProcess &p);
+
     // Helpers to find binaries. Do not use it for other path variables
     // and file types.
     static QString locateBinary(const QString &binary);

src/plugins/git/gitclient.cpp

@@ -49,6 +49,7 @@
 
 #include <texteditor/itexteditor.h>
 #include <utils/qtcassert.h>
+#include <utils/synchronousprocess.h>
 #include <vcsbase/vcsbaseeditor.h>
 #include <vcsbase/vcsbaseoutputwindow.h>
 
@@ -1042,22 +1043,16 @@ bool GitClient::synchronousGit(const QString &workingDirectory,
         return false;
     }
 
-    if (!process.waitForFinished(m_settings.timeoutSeconds * 1000)) {
-        if (errorText)
-            *errorText = GitCommand::msgTimeout(m_settings.timeoutSeconds).toLocal8Bit();
-        GitCommand::stopProcess(process);
+    if (!Utils::SynchronousProcess::readDataFromProcess(process, m_settings.timeoutSeconds * 1000,
+                                                        outputText, errorText)) {
+        *errorText->append(GitCommand::msgTimeout(m_settings.timeoutSeconds).toLocal8Bit());
+        Utils::SynchronousProcess::stopProcess(process);
         return false;
     }
 
-    if (outputText)
-        *outputText = process.readAllStandardOutput();
-
-    if (errorText)
-        *errorText = process.readAllStandardError();
-
     if (Git::Constants::debug)
-        qDebug() << "synchronousGit ex=" << process.exitCode();
-    return process.exitCode() == 0;
+        qDebug() << "synchronousGit ex=" << process.exitStatus() << process.exitCode();
+    return process.exitStatus() == QProcess::NormalExit && process.exitCode() == 0;
 }
 
 static inline int
@@ -1546,7 +1541,8 @@ QString GitClient::readConfig(const QString &workingDirectory, const QStringList
     arguments << QLatin1String("config") << configVar;
 
     QByteArray outputText;
-    if (synchronousGit(workingDirectory, arguments, &outputText, 0, false))
+    QByteArray errorText;
+    if (synchronousGit(workingDirectory, arguments, &outputText, &errorText, false))
         return commandOutputFromLocal8Bit(outputText);
     return QString();
 }

src/plugins/git/gitclient.h

@@ -230,8 +230,8 @@ private:
 
     bool synchronousGit(const QString &workingDirectory,
                         const QStringList &arguments,
-                        QByteArray* outputText = 0,
-                        QByteArray* errorText = 0,
+                        QByteArray* outputText,
+                        QByteArray* errorText,
                         bool logCommandToWindow = true);
     // determine version as '(major << 16) + (minor << 8) + patch' or 0.
     unsigned synchronousGitVersion(bool silent, QString *errorMessage = 0);

src/plugins/git/gitcommand.cpp

@@ -33,6 +33,7 @@
 #include <coreplugin/icore.h>
 #include <coreplugin/progressmanager/progressmanager.h>
 #include <extensionsystem/pluginmanager.h>
+#include <utils/synchronousprocess.h>
 
 #include <QtCore/QDebug>
 #include <QtCore/QProcess>
@@ -117,17 +118,6 @@ QString GitCommand::msgTimeout(int seconds)
     return tr("Error: Git timed out after %1s.").arg(seconds);
 }
 
-bool GitCommand::stopProcess(QProcess &p)
-{
-    if (p.state() != QProcess::Running)
-        return true;
-    p.terminate();
-    if (p.waitForFinished(300))
-        return true;
-    p.kill();
-    return p.waitForFinished(300);
-}
-
 void GitCommand::run()
 {
     if (Git::Constants::debug)
@@ -138,7 +128,8 @@ void GitCommand::run()
 
     process.setEnvironment(m_environment);
 
-    QByteArray output;
+    QByteArray stdOut;
+    QByteArray stdErr;
     QString error;
 
     const int count = m_jobs.size();
@@ -156,20 +147,20 @@ void GitCommand::run()
 
         process.closeWriteChannel();
         const int timeOutSeconds = m_jobs.at(j).timeout;
-        if (!process.waitForFinished(timeOutSeconds * 1000)) {
-            stopProcess(process);
+        if (!Utils::SynchronousProcess::readDataFromProcess(process, timeOutSeconds * 1000,
+                                                            &stdOut, &stdErr)) {
+            Utils::SynchronousProcess::stopProcess(process);
             ok = false;
             error += msgTimeout(timeOutSeconds);
             break;
         }
 
-        output += process.readAllStandardOutput();
-        error += QString::fromLocal8Bit(process.readAllStandardError());
+        error += QString::fromLocal8Bit(stdErr);
         switch (m_reportTerminationMode) {
         case NoReport:
             break;
         case ReportStdout:
-            output += msgTermination(process.exitCode(), m_binaryPath, m_jobs.at(j).arguments).toUtf8();
+            stdOut += msgTermination(process.exitCode(), m_binaryPath, m_jobs.at(j).arguments).toUtf8();
             break;
         case ReportStderr:
             error += msgTermination(process.exitCode(), m_binaryPath, m_jobs.at(j).arguments);
@@ -178,16 +169,16 @@ void GitCommand::run()
     }
 
     // Special hack: Always produce output for diff
-    if (ok && output.isEmpty() && m_jobs.front().arguments.at(0) == QLatin1String("diff")) {
-        output += "The file does not differ from HEAD";
+    if (ok && stdOut.isEmpty() && m_jobs.front().arguments.at(0) == QLatin1String("diff")) {
+        stdOut += "The file does not differ from HEAD";
     } else {
         // @TODO: Remove, see below
         if (ok && m_jobs.front().arguments.at(0) == QLatin1String("status"))
-            removeColorCodes(&output);
+            removeColorCodes(&stdOut);
     }
 
-    if (ok && !output.isEmpty())
-        emit outputData(output);
+    if (ok && !stdOut.isEmpty())
+        emit outputData(stdOut);
 
     if (!error.isEmpty())
         emit errorText(error);

src/plugins/git/gitcommand.h

@@ -71,8 +71,6 @@ public:
     void setTerminationReportMode(TerminationReportMode m);
 
     static QString msgTimeout(int seconds);
-    // Helper to kill a process by SIGNAL first, allowing for cleanup
-    static bool stopProcess(QProcess &p);
 
 private:
     void run();

src/plugins/mercurial/mercurialclient.cpp

@@ -37,6 +37,7 @@
 #include <coreplugin/editormanager/editormanager.h>
 
 #include <utils/qtcassert.h>
+#include <utils/synchronousprocess.h>
 #include <vcsbase/vcsbaseeditor.h>
 #include <vcsbase/vcsbaseoutputwindow.h>
 
@@ -82,14 +83,16 @@ bool MercurialClient::add(const QString &workingDir, const QString &filename)
 {
     QStringList args;
     args << QLatin1String("add") << filename;
-    return executeHgSynchronously(workingDir, args);
+    QByteArray stdOut;
+    return executeHgSynchronously(workingDir, args, &stdOut);
 }
 
 bool MercurialClient::remove(const QString &workingDir, const QString &filename)
 {
     QStringList args;
     args << QLatin1String("remove") << filename;
-    return executeHgSynchronously(workingDir, args);
+    QByteArray stdOut;
+    return executeHgSynchronously(workingDir, args, &stdOut);
 }
 
 bool MercurialClient::manifestSync(const QString &repository, const QString &relativeFilename)
@@ -135,19 +138,17 @@ bool MercurialClient::executeHgSynchronously(const QString  &workingDir,
 
     hgProcess.closeWriteChannel();
 
-    if (!hgProcess.waitForFinished(settings.timeoutMilliSeconds())) {
-        hgProcess.terminate();
+    QByteArray stdErr;
+    if (!Utils::SynchronousProcess::readDataFromProcess(hgProcess, settings.timeoutMilliSeconds(),
+                                                        output, &stdErr)) {
+        Utils::SynchronousProcess::stopProcess(hgProcess);
         outputWindow->appendError(MercurialJobRunner::msgTimeout(settings.timeoutSeconds()));
         return false;
     }
+    if (!stdErr.isEmpty())
+        outputWindow->append(QString::fromLocal8Bit(stdErr));
 
-    if ((hgProcess.exitStatus() == QProcess::NormalExit) && (hgProcess.exitCode() == 0)) {
-        if (output)
-            *output = hgProcess.readAllStandardOutput();
-        return true;
-    }
-
-    return false;
+    return hgProcess.exitStatus() == QProcess::NormalExit && hgProcess.exitCode() == 0;
 }
 
 QString MercurialClient::branchQuerySync(const QString &repositoryRoot)

src/plugins/mercurial/mercurialclient.h

@@ -114,7 +114,7 @@ private slots:
 private:
     bool executeHgSynchronously(const QString  &workingDir,
                                 const QStringList &args,
-                                QByteArray *output=0) const;
+                                QByteArray *output) const;
     void enqueueJob(const QSharedPointer<HgTask> &);
     void revert(const QString &workingDir, const QString &argument,
                 const QString &revision, const QVariant &cookie);

src/plugins/mercurial/mercurialjobrunner.cpp

@@ -34,6 +34,7 @@
 
 #include <vcsbase/vcsbaseoutputwindow.h>
 #include <vcsbase/vcsbaseeditor.h>
+#include <utils/synchronousprocess.h>
 
 #include <QtCore/QProcess>
 #include <QtCore/QString>
@@ -112,7 +113,7 @@ void MercurialJobRunner::getSettings()
 {
     const MercurialSettings &settings = MercurialPlugin::instance()->settings();
     binary = settings.binary();
-    timeout = settings.timeoutMilliSeconds();
+    m_timeoutMS = settings.timeoutMilliSeconds();
     standardArguments = settings.standardArguments();
 }
 
@@ -203,25 +204,27 @@ void MercurialJobRunner::task(const QSharedPointer<HgTask> &job)
 
     hgProcess.closeWriteChannel();
 
-    if (!hgProcess.waitForFinished(timeout)) {
-        hgProcess.terminate();
-        emit error(msgTimeout(timeout / 1000));
+    QByteArray stdOutput;
+    QByteArray stdErr;
+
+    if (!Utils::SynchronousProcess::readDataFromProcess(hgProcess, m_timeoutMS, &stdOutput, &stdErr)) {
+        Utils::SynchronousProcess::stopProcess(hgProcess);
+        emit error(msgTimeout(m_timeoutMS / 1000));
         return;
     }
 
-    if ((hgProcess.exitStatus() == QProcess::NormalExit) && (hgProcess.exitCode() == 0)) {
-        QByteArray stdOutput= hgProcess.readAllStandardOutput();
+    if (hgProcess.exitStatus() == QProcess::NormalExit && hgProcess.exitCode() == 0) {
         /*
           * sometimes success means output is actually on error channel (stderr)
           * e.g. "hg revert" outputs "no changes needed to 'file'" on stderr if file has not changed
           * from revision specified
           */
         if (stdOutput.isEmpty())
-            stdOutput = hgProcess.readAllStandardError();
+            stdOutput = stdErr;
         emit output(stdOutput);
         taskData->emitSucceeded();
     } else {
-        emit error(QString::fromLocal8Bit(hgProcess.readAllStandardError()));
+        emit error(QString::fromLocal8Bit(stdErr));
     }
 
     hgProcess.close();

src/plugins/mercurial/mercurialjobrunner.h

@@ -115,7 +115,7 @@ private:
     bool keepRunning;
     QString binary;
     QStringList standardArguments;
-    int timeout;
+    int m_timeoutMS;
 };
 
 } //namespace Internal

src/plugins/perforce/perforcechecker.cpp

@@ -30,6 +30,7 @@
 #include "perforcechecker.h"
 
 #include <utils/qtcassert.h>
+#include <utils/synchronousprocess.h>
 
 #include <QtCore/QRegExp>
 #include <QtCore/QTimer>
@@ -101,23 +102,12 @@ void PerforceChecker::start(const QString &binary,
     }
 }
 
-bool PerforceChecker::ensureProcessStopped(QProcess &p)
-{
-    if (p.state() != QProcess::Running)
-        return true;
-    p.terminate();
-    if (p.waitForFinished(300))
-        return true;
-    p.kill();
-    return p.waitForFinished(300);
-}
-
 void PerforceChecker::slotTimeOut()
 {
     if (!isRunning())
         return;
     m_timedOut = true;
-    ensureProcessStopped(m_process);
+    Utils::SynchronousProcess::stopProcess(m_process);
     emitFailed(tr("\"%1\" timed out after %2ms.").arg(m_binary).arg(m_timeOutMS));
 }
 
@@ -135,7 +125,7 @@ void PerforceChecker::slotError(QProcess::ProcessError error)
     case QProcess::ReadError:
     case QProcess::WriteError:
     case QProcess::UnknownError:
-        ensureProcessStopped(m_process);
+        Utils::SynchronousProcess::stopProcess(m_process);
         break;
     }
 }

src/plugins/perforce/perforcechecker.h

@@ -57,8 +57,6 @@ public slots:
     bool useOverideCursor() const;
     void setUseOverideCursor(bool v);
 
-    static bool ensureProcessStopped(QProcess &p);
-
 signals:
     void succeeded(const QString &repositoryRoot);
     void failed(const QString &errorMessage);

src/plugins/perforce/perforceplugin.cpp

@@ -1075,7 +1075,7 @@ PerforceResponse PerforcePlugin::fullySynchronousProcess(const QString &workingD
     }
     if (!stdInput.isEmpty()) {
         if (process.write(stdInput) == -1) {
-            PerforceChecker::ensureProcessStopped(process);
+            Utils::SynchronousProcess::stopProcess(process);
             response.error = true;
             response.message = tr("Unable to write input data to process %1: %2").arg(m_settings.p4Command(), process.errorString());
             return response;
@@ -1083,9 +1083,11 @@ PerforceResponse PerforcePlugin::fullySynchronousProcess(const QString &workingD
         process.closeWriteChannel();
     }
 
+    QByteArray stdOut;
+    QByteArray stdErr;
     const int timeOut = (flags & LongTimeOut) ? m_settings.longTimeOutMS() : m_settings.timeOutMS();
-    if (!process.waitForFinished(timeOut)) {
-        PerforceChecker::ensureProcessStopped(process);
+    if (!Utils::SynchronousProcess::readDataFromProcess(process, timeOut, &stdOut, &stdErr)) {
+        Utils::SynchronousProcess::stopProcess(process);
         response.error = true;
         response.message = msgTimeout(timeOut);
         return response;
@@ -1097,8 +1099,7 @@ PerforceResponse PerforcePlugin::fullySynchronousProcess(const QString &workingD
     }
     response.exitCode = process.exitCode();
     response.error = response.exitCode ? !(flags & IgnoreExitCode) : false;
-    response.stdErr = QString::fromLocal8Bit(process.readAllStandardError());
-    const QByteArray stdOut = process.readAllStandardOutput();
+    response.stdErr = QString::fromLocal8Bit(stdErr);
     response.stdOut = outputCodec ? outputCodec->toUnicode(stdOut.constData(), stdOut.size()) :
                                     QString::fromLocal8Bit(stdOut);
     const QChar cr = QLatin1Char('\r');

src/plugins/vcsbase/checkoutjobs.cpp

@@ -30,6 +30,7 @@
 #include "checkoutjobs.h"
 
 #include <QtCore/QDebug>
+#include <utils/synchronousprocess.h>
 
 enum { debug = 0 };
 namespace VCSBase {
@@ -137,9 +138,7 @@ void ProcessCheckoutJob::cancel()
         qDebug() << "ProcessCheckoutJob::start";
 
     emit output(tr("Stopping..."));
-    d->process.terminate();
-    if (!d->process.waitForFinished(5000))
-        d->process.kill();
+    Utils::SynchronousProcess::stopProcess(d->process);
 }
 
 } // namespace VCSBase

src/plugins/vcsbase/vcsbasesubmiteditor.cpp

@@ -42,6 +42,7 @@
 #include <coreplugin/actionmanager/actionmanager.h>
 #include <utils/submiteditorwidget.h>
 #include <utils/checkablemessagebox.h>
+#include <utils/synchronousprocess.h>
 #include <utils/submitfieldwidget.h>
 #include <find/basetextfind.h>
 #include <texteditor/fontsettings.h>
@@ -580,18 +581,20 @@ bool VCSBaseSubmitEditor::runSubmitMessageCheckScript(const QString &checkScript
         *errorMessage = tr("The check script '%1' could not be started: %2").arg(checkScript, checkProcess.errorString());
         return false;
     }
-    if (!checkProcess.waitForFinished()) {
-        *errorMessage = tr("The check script '%1' could not be run: %2").arg(checkScript, checkProcess.errorString());
+    QByteArray stdOutData;
+    QByteArray stdErrData;
+    if (!Utils::SynchronousProcess::readDataFromProcess(checkProcess, 30000, &stdOutData, &stdErrData)) {
+        Utils::SynchronousProcess::stopProcess(checkProcess);
+        *errorMessage = tr("The check script '%1' timed out.").arg(checkScript);
         return false;
     }
     if (checkProcess.exitStatus() != QProcess::NormalExit) {
         *errorMessage = tr("The check script '%1' crashed").arg(checkScript);
         return false;
     }
-    const QString stdOut = QString::fromLocal8Bit(checkProcess.readAllStandardOutput());
-    if (!stdOut.isEmpty())
-        outputWindow->appendSilently(stdOut);
-    const QString stdErr = QString::fromLocal8Bit(checkProcess.readAllStandardError());
+    if (!stdOutData.isEmpty())
+        outputWindow->appendSilently(QString::fromLocal8Bit(stdOutData));
+    const QString stdErr = QString::fromLocal8Bit(stdErrData);
     if (!stdErr.isEmpty())
         outputWindow->appendSilently(stdErr);
     const int exitCode = checkProcess.exitCode();