QProcess/Win: avoid double buffering on write

As QWindowsPipeWriter now maintains a chunk queue, there is no need to
use the internal QIODevice buffer and wait for the previous operation
to complete.

This also allows us to get rid of the stdinWriteTrigger timer; however,
as a trade-off, QWindowsPipeWriter now needs to accept data even before
a handle is assigned.

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

src/corelib/io/qprocess.cpp

@@ -782,7 +782,9 @@ void QProcessPrivate::Channel::clear()
 QProcessPrivate::QProcessPrivate()
 {
     readBufferChunkSize = QRINGBUFFER_CHUNKSIZE;
+#ifndef Q_OS_WIN
     writeBufferChunkSize = QRINGBUFFER_CHUNKSIZE;
+#endif
 }
 
 /*!
@@ -1585,11 +1587,11 @@ bool QProcess::isSequential() const
 */
 qint64 QProcess::bytesToWrite() const
 {
-    qint64 size = QIODevice::bytesToWrite();
 #ifdef Q_OS_WIN
-    size += d_func()->pipeWriterBytesToWrite();
+    return d_func()->pipeWriterBytesToWrite();
+#else
+    return QIODevice::bytesToWrite();
 #endif
-    return size;
 }
 
 /*!

src/corelib/io/qprocess.h

@@ -281,7 +281,9 @@ private:
 
     Q_PRIVATE_SLOT(d_func(), bool _q_canReadStandardOutput())
     Q_PRIVATE_SLOT(d_func(), bool _q_canReadStandardError())
+#ifdef Q_OS_UNIX
     Q_PRIVATE_SLOT(d_func(), bool _q_canWrite())
+#endif
     Q_PRIVATE_SLOT(d_func(), bool _q_startupNotification())
     Q_PRIVATE_SLOT(d_func(), void _q_processDied())
 };

src/corelib/io/qprocess_p.h

@@ -81,7 +81,6 @@ class QSocketNotifier;
 class QWindowsPipeReader;
 class QWindowsPipeWriter;
 class QWinEventNotifier;
-class QTimer;
 
 #ifdef Q_OS_WIN
 class QProcEnvKey : public QString
@@ -286,9 +285,12 @@ public:
     bool _q_canReadStandardOutput();
     bool _q_canReadStandardError();
 #ifdef Q_OS_WIN
+    qint64 pipeWriterBytesToWrite() const;
     void _q_bytesWritten(qint64 bytes);
-#endif
+#else
     bool _q_canWrite();
+    bool writeToStdin();
+#endif
     bool _q_startupNotification();
     void _q_processDied();
 
@@ -335,7 +337,6 @@ public:
     QSocketNotifier *stateNotifier = nullptr;
     int forkfd = -1;
 #else
-    QTimer *stdinWriteTrigger = nullptr;
     QWinEventNotifier *processFinishedNotifier = nullptr;
 #endif
 
@@ -357,7 +358,6 @@ public:
     STARTUPINFOW createStartupInfo();
     bool callCreateProcess(QProcess::CreateProcessArguments *cpargs);
     bool drainOutputPipes();
-    qint64 pipeWriterBytesToWrite() const;
 #endif
 
     bool startDetached(qint64 *pPid);
@@ -373,7 +373,6 @@ public:
 
     qint64 bytesAvailableInChannel(const Channel *channel) const;
     qint64 readFromChannel(const Channel *channel, char *data, qint64 maxlen);
-    bool writeToStdin();
 
     void destroyPipe(Q_PIPE pipe[2]);
     void cleanup();

src/corelib/io/qprocess_win.cpp

@@ -53,7 +53,6 @@
 #include <qrandom.h>
 #include <qwineventnotifier.h>
 #include <qscopedvaluerollback.h>
-#include <qtimer.h>
 #include <private/qsystemlibrary_p.h>
 #include <private/qthread_p.h>
 
@@ -376,8 +375,6 @@ void QProcessPrivate::cleanup()
     q_func()->setProcessState(QProcess::NotRunning);
 
     closeChannels();
-    delete stdinWriteTrigger;
-    stdinWriteTrigger = nullptr;
     delete processFinishedNotifier;
     processFinishedNotifier = nullptr;
     if (pid) {
@@ -618,6 +615,12 @@ void QProcessPrivate::startProcess()
         return;
     }
 
+    // The pipe writer may have already been created before we had
+    // the pipe handle, specifically if the user wrote data from the
+    // stateChanged() slot.
+    if (stdinChannel.writer)
+        stdinChannel.writer->setHandle(stdinChannel.pipe[1]);
+
     q->setProcessState(QProcess::Running);
     // User can call kill()/terminate() from the stateChanged() slot
     // so check before proceeding
@@ -713,9 +716,6 @@ bool QProcessPrivate::drainOutputPipes()
 bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline)
 {
     forever {
-        if (!writeBuffer.isEmpty() && !_q_canWrite())
-            return false;
-
         QProcessPoller poller(*this);
         int ret = poller.poll(deadline);
         if (ret < 0)
@@ -748,10 +748,11 @@ bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline)
 bool QProcessPrivate::waitForBytesWritten(const QDeadlineTimer &deadline)
 {
     forever {
-        // If no write is pending, try to start one. However, at entry into
-        // the loop the write buffer can be empty to start with, in which
-        // case _q_caWrite() fails immediately.
-        if (pipeWriterBytesToWrite() == 0 && !_q_canWrite())
+        // At entry into the loop the pipe writer's buffer can be empty to
+        // start with, in which case we fail immediately. Also, if the input
+        // pipe goes down somewhere in the code below, we avoid waiting for
+        // a full timeout.
+        if (pipeWriterBytesToWrite() == 0)
             return false;
 
         QProcessPoller poller(*this);
@@ -797,9 +798,6 @@ bool QProcessPrivate::waitForFinished(const QDeadlineTimer &deadline)
 #endif
 
     forever {
-        if (!writeBuffer.isEmpty() && !_q_canWrite())
-            return false;
-
         QProcessPoller poller(*this);
         int ret = poller.poll(deadline);
         if (ret < 0)
@@ -854,16 +852,16 @@ qint64 QProcess::writeData(const char *data, qint64 len)
         return 0;
     }
 
-    if (!d->stdinWriteTrigger) {
-        d->stdinWriteTrigger = new QTimer;
-        d->stdinWriteTrigger->setSingleShot(true);
-        QObjectPrivate::connect(d->stdinWriteTrigger, &QTimer::timeout,
-                                d, &QProcessPrivate::_q_canWrite);
+    if (!d->stdinChannel.writer) {
+        d->stdinChannel.writer = new QWindowsPipeWriter(d->stdinChannel.pipe[1], this);
+        QObjectPrivate::connect(d->stdinChannel.writer, &QWindowsPipeWriter::bytesWritten,
+                                d, &QProcessPrivate::_q_bytesWritten);
     }
 
-    d->write(data, len);
-    if (!d->stdinWriteTrigger->isActive())
-        d->stdinWriteTrigger->start();
+    if (d->isWriteChunkCached(data, len))
+        d->stdinChannel.writer->write(*(d->currentWriteChunk));
+    else
+        d->stdinChannel.writer->write(data, len);
 
 #if defined QPROCESS_DEBUG
     qDebug("QProcess::writeData(%p \"%s\", %lld) == %lld (written to buffer)",
@@ -885,38 +883,8 @@ void QProcessPrivate::_q_bytesWritten(qint64 bytes)
         QScopedValueRollback<bool> guard(emittedBytesWritten, true);
         emit q->bytesWritten(bytes);
     }
-    _q_canWrite();
-}
-
-bool QProcessPrivate::_q_canWrite()
-{
-    if (writeBuffer.isEmpty()) {
-        if (stdinChannel.closed && pipeWriterBytesToWrite() == 0)
-            closeWriteChannel();
-#if defined QPROCESS_DEBUG
-        qDebug("QProcessPrivate::canWrite(), not writing anything (empty write buffer).");
-#endif
-        return false;
-    }
-
-    return writeToStdin();
-}
-
-bool QProcessPrivate::writeToStdin()
-{
-    Q_Q(QProcess);
-
-    if (!stdinChannel.writer) {
-        stdinChannel.writer = new QWindowsPipeWriter(stdinChannel.pipe[1], q);
-        QObjectPrivate::connect(stdinChannel.writer, &QWindowsPipeWriter::bytesWritten,
-                                this, &QProcessPrivate::_q_bytesWritten);
-    } else {
-        if (stdinChannel.writer->isWriteOperationActive())
-            return true;
-    }
-
-    stdinChannel.writer->write(writeBuffer.read());
-    return true;
+    if (stdinChannel.closed && pipeWriterBytesToWrite() == 0)
+        closeWriteChannel();
 }
 
 // Use ShellExecuteEx() to trigger an UAC prompt when CreateProcess()fails

src/corelib/io/qwindowspipewriter.cpp

@@ -72,6 +72,19 @@ QWindowsPipeWriter::~QWindowsPipeWriter()
     CloseHandle(syncHandle);
 }
 
+/*!
+    Assigns the handle to this writer. The handle must be valid.
+    Call this function if data was buffered before getting the handle.
+ */
+void QWindowsPipeWriter::setHandle(HANDLE hPipeWriteEnd)
+{
+    Q_ASSERT(!stopped);
+
+    handle = hPipeWriteEnd;
+    QMutexLocker locker(&mutex);
+    startAsyncWriteLocked(&locker);
+}
+
 /*!
     Stops the asynchronous write sequence.
     If the write sequence is running then the I/O operation is canceled.
@@ -150,6 +163,12 @@ inline bool QWindowsPipeWriter::writeImpl(Args... args)
         return true;
 
     stopped = false;
+
+    // If we don't have an assigned handle yet, defer writing until
+    // setHandle() is called.
+    if (handle == INVALID_HANDLE_VALUE)
+        return true;
+
     startAsyncWriteLocked(&locker);
     return true;
 }

src/corelib/io/qwindowspipewriter_p.h

@@ -68,6 +68,7 @@ public:
     explicit QWindowsPipeWriter(HANDLE pipeWriteEnd, QObject *parent = nullptr);
     ~QWindowsPipeWriter();
 
+    void setHandle(HANDLE hPipeWriteEnd);
     bool write(const QByteArray &ba);
     bool write(const char *data, qint64 size);
     void stop();