QProcess/Win: move pipe draining into QWindowsPipeReader

... where it belongs.

To avoid the loop, introduce the drainAndStop() function, which allows
QWindowsPipeReader to flush the pipe itself. It determines the number of
bytes pending and blocks until the remainder of the process output is
received.

Note that the loop in drainOutputPipes() didn't actually have to
interleave the two pipes (because we're presuming that the operations
will finish instantly), so we don't do it now. Also, the code violated
the API contract: 'true' was returned when the 'wrong' channel received
data; this is now fixed as a side effect.

Change-Id: I38ed4861a238e39e793c3716e856e5bfdeed3d74
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

4 files changed, 89 insertions, 32 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 9170b72e56..05aa0f021f 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -1142,9 +1142,10 @@ void QProcessPrivate::_q_processDied()
     // so the data is made available before we announce death.
 #ifdef Q_OS_WIN
     drainOutputPipes();
-#endif
+#else
     _q_canReadStandardOutput();
     _q_canReadStandardError();
+#endif
 
     // Slots connected to signals emitted by the functions called above
     // might call waitFor*(), which would synchronously reap the process.
diff --git a/src/corelib/io/qprocess_win.cpp b/src/corelib/io/qprocess_win.cpp
index e65d4683be..dc8c50127d 100644
--- a/src/corelib/io/qprocess_win.cpp
+++ b/src/corelib/io/qprocess_win.cpp
@@ -642,28 +642,18 @@ bool QProcessPrivate::waitForStarted(const QDeadlineTimer &)
 
 bool QProcessPrivate::drainOutputPipes()
 {
-    if (!stdoutChannel.reader && !stderrChannel.reader)
-        return false;
+    bool readyReadEmitted = false;
 
-    bool someReadyReadEmitted = false;
-    forever {
-        bool readyReadEmitted = false;
-        bool readOperationActive = false;
-        if (stdoutChannel.reader) {
-            readyReadEmitted |= stdoutChannel.reader->waitForReadyRead(0);
-            readOperationActive = stdoutChannel.reader && stdoutChannel.reader->isReadOperationActive();
-        }
-        if (stderrChannel.reader) {
-            readyReadEmitted |= stderrChannel.reader->waitForReadyRead(0);
-            readOperationActive |= stderrChannel.reader && stderrChannel.reader->isReadOperationActive();
-        }
-        someReadyReadEmitted |= readyReadEmitted;
-        if (!readOperationActive || !readyReadEmitted)
-            break;
-        QThread::yieldCurrentThread();
+    if (stdoutChannel.reader) {
+        stdoutChannel.reader->drainAndStop();
+        readyReadEmitted = _q_canReadStandardOutput();
+    }
+    if (stderrChannel.reader) {
+        stderrChannel.reader->drainAndStop();
+        readyReadEmitted |= _q_canReadStandardError();
     }
 
-    return someReadyReadEmitted;
+    return readyReadEmitted;
 }
 
 bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline)
diff --git a/src/corelib/io/qwindowspipereader.cpp b/src/corelib/io/qwindowspipereader.cpp
index c20909766d..b525e88282 100644
--- a/src/corelib/io/qwindowspipereader.cpp
+++ b/src/corelib/io/qwindowspipereader.cpp
@@ -44,6 +44,8 @@
 
 QT_BEGIN_NAMESPACE
 
+static const DWORD minReadBufferSize = 4096;
+
 QWindowsPipeReader::Overlapped::Overlapped(QWindowsPipeReader *reader)
     : pipeReader(reader)
 {
@@ -61,7 +63,8 @@ QWindowsPipeReader::QWindowsPipeReader(QObject *parent)
       overlapped(this),
       readBufferMaxSize(0),
       actualReadBufferSize(0),
-      stopped(true),
+      bytesPending(0),
+      state(Stopped),
       readSequenceStarted(false),
       notifiedCalled(false),
       pipeBroken(false),
@@ -84,6 +87,7 @@ void QWindowsPipeReader::setHandle(HANDLE hPipeReadEnd)
 {
     readBuffer.clear();
     actualReadBufferSize = 0;
+    bytesPending = 0;
     handle = hPipeReadEnd;
     pipeBroken = false;
 }
@@ -94,7 +98,25 @@ void QWindowsPipeReader::setHandle(HANDLE hPipeReadEnd)
  */
 void QWindowsPipeReader::stop()
 {
-    stopped = true;
+    state = Stopped;
+    cancelAsyncRead();
+}
+
+/*!
+    Stops the asynchronous read sequence.
+    Reads all pending bytes into the internal buffer.
+ */
+void QWindowsPipeReader::drainAndStop()
+{
+    state = Draining;
+    cancelAsyncRead();
+}
+
+/*!
+    Stops the asynchronous read sequence.
+ */
+void QWindowsPipeReader::cancelAsyncRead()
+{
     if (readSequenceStarted) {
         if (!CancelIoEx(handle, &overlapped)) {
             const DWORD dwError = GetLastError();
@@ -135,7 +157,7 @@ qint64 QWindowsPipeReader::read(char *data, qint64 maxlen)
     }
 
     if (!pipeBroken) {
-        if (!readSequenceStarted && !stopped)
+        if (state == Running)
             startAsyncRead();
         if (readSoFar == 0)
             return -2;      // signal EWOULDBLOCK
@@ -171,7 +193,7 @@ void QWindowsPipeReader::notified(DWORD errorCode, DWORD numberOfBytesRead)
         pipeBroken = true;
         break;
     case ERROR_OPERATION_ABORTED:
-        if (stopped)
+        if (state != Running)
             break;
         Q_FALLTHROUGH();
     default:
@@ -183,16 +205,34 @@ void QWindowsPipeReader::notified(DWORD errorCode, DWORD numberOfBytesRead)
     // After the reader was stopped, the only reason why this function can be called is the
     // completion of a cancellation. No signals should be emitted, and no new read sequence should
     // be started in this case.
-    if (stopped)
+    if (state == Stopped)
         return;
 
     if (pipeBroken) {
-        emit pipeClosed();
+        emitPipeClosed();
         return;
     }
 
     actualReadBufferSize += numberOfBytesRead;
     readBuffer.truncate(actualReadBufferSize);
+
+    // Read all pending data from the pipe's buffer in 'Draining' state.
+    if (state == Draining) {
+        // Determine the number of pending bytes on the first iteration.
+        if (bytesPending == 0)
+            bytesPending = checkPipeState();
+        else
+            bytesPending -= numberOfBytesRead;
+
+        if (bytesPending == 0) // all data received
+            return; // unblock waitForNotification() in cancelAsyncRead()
+
+        startAsyncReadHelper(bytesPending);
+        if (readSequenceStarted)
+            notifiedCalled = false; // wait for more data
+        return;
+    }
+
     startAsyncRead();
     if (!readyReadPending) {
         readyReadPending = true;
@@ -202,12 +242,25 @@ void QWindowsPipeReader::notified(DWORD errorCode, DWORD numberOfBytesRead)
 
 /*!
     \internal
-    Reads data from the pipe into the readbuffer.
+    Starts an asynchronous read sequence on the pipe.
  */
 void QWindowsPipeReader::startAsyncRead()
 {
-    const DWORD minReadBufferSize = 4096;
-    qint64 bytesToRead = qMax(checkPipeState(), minReadBufferSize);
+    if (readSequenceStarted)
+        return;
+
+    state = Running;
+    startAsyncReadHelper(qMax(checkPipeState(), minReadBufferSize));
+}
+
+/*!
+    \internal
+    Starts a new read sequence.
+ */
+void QWindowsPipeReader::startAsyncReadHelper(qint64 bytesToRead)
+{
+    Q_ASSERT(bytesToRead != 0);
+
     if (pipeBroken)
         return;
 
@@ -222,7 +275,6 @@ void QWindowsPipeReader::startAsyncRead()
 
     char *ptr = readBuffer.reserve(bytesToRead);
 
-    stopped = false;
     readSequenceStarted = true;
     overlapped.clear();
     if (!ReadFileEx(handle, ptr, bytesToRead, &overlapped, &readFileCompleted)) {
@@ -267,7 +319,7 @@ DWORD QWindowsPipeReader::checkPipeState()
         return bytes;
     if (!pipeBroken) {
         pipeBroken = true;
-        emit pipeClosed();
+        emitPipeClosed();
     }
     return 0;
 }
@@ -299,6 +351,14 @@ void QWindowsPipeReader::emitPendingReadyRead()
     }
 }
 
+void QWindowsPipeReader::emitPipeClosed()
+{
+    // We are not allowed to emit signals in either 'Stopped'
+    // or 'Draining' state.
+    if (state == Running)
+        emit pipeClosed();
+}
+
 /*!
     Waits for the completion of the asynchronous read operation.
     Returns \c true, if we've emitted the readyRead signal (non-recursive case)
diff --git a/src/corelib/io/qwindowspipereader_p.h b/src/corelib/io/qwindowspipereader_p.h
index 2842343597..c61018d87d 100644
--- a/src/corelib/io/qwindowspipereader_p.h
+++ b/src/corelib/io/qwindowspipereader_p.h
@@ -68,6 +68,7 @@ public:
     void setHandle(HANDLE hPipeReadEnd);
     void startAsyncRead();
     void stop();
+    void drainAndStop();
 
     void setMaxReadBufferSize(qint64 size) { readBufferMaxSize = size; }
     qint64 maxReadBufferSize() const { return readBufferMaxSize; }
@@ -88,12 +89,15 @@ Q_SIGNALS:
     void _q_queueReadyRead(QPrivateSignal);
 
 private:
+    void startAsyncReadHelper(qint64 bytesToRead);
+    void cancelAsyncRead();
     static void CALLBACK readFileCompleted(DWORD errorCode, DWORD numberOfBytesTransfered,
                                            OVERLAPPED *overlappedBase);
     void notified(DWORD errorCode, DWORD numberOfBytesRead);
     DWORD checkPipeState();
     bool waitForNotification(int timeout);
     void emitPendingReadyRead();
+    void emitPipeClosed();
 
     class Overlapped : public OVERLAPPED
     {
@@ -109,7 +113,9 @@ private:
     qint64 readBufferMaxSize;
     QRingBuffer readBuffer;
     qint64 actualReadBufferSize;
-    bool stopped;
+    qint64 bytesPending;
+
+    enum State { Stopped, Running, Draining } state;
     bool readSequenceStarted;
     bool notifiedCalled;
     bool pipeBroken;