From 3ccfc351fdcbb117e2872229382e45a929dac62a Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 5 Jun 2014 16:44:07 -0700 Subject: [PATCH] QProcess: Handle spurious socket notifications for stdout and stderr On Unix systems where the GUI event dispatcher uses a notification system for socket notifiers that is out of band compared to select(), it's possible for the QSocketNotifier to activate after the pipe has been read from. When that happened, the ioctl(2) call with FIONREAD might return 0 bytes available, which we interpreted to mean EOF. Instead of doing that, always try to read at least one byte and examine the returned byte count from read(2). If it returns 0, that's a real EOF; if it returns -1 EWOULDBLOCK, we simply ignore the situation. That's the case on OS X: the Cocoa event dispatcher uses CFSocket to get notifications and those use kevent (and, apparently, an auxiliary thread) instead of an in-thread select() or poll(). That means the event loop would activate the QSocketNotifier even though there is nothing to be read. Task-number: QTBUG-39488 Change-Id: I1a58b5b1db7a47034fb36a78a005ebff96290efb Reviewed-by: Oswald Buddenhagen --- src/corelib/io/qprocess.cpp | 28 ++++-- src/corelib/io/qprocess_unix.cpp | 4 + src/corelib/io/qwindowspipereader.cpp | 4 +- src/network/socket/qlocalsocket_win.cpp | 12 ++- tests/auto/corelib/io/qprocess/test/test.pro | 1 - .../auto/corelib/io/qprocess/tst_qprocess.cpp | 2 +- tests/auto/other/other.pro | 3 + .../qprocess_and_guieventloop.pro | 4 + .../other/qprocess_and_guieventloop/test.pro | 5 ++ .../tst_qprocess_and_guieventloop.cpp | 89 +++++++++++++++++++ .../write-read-write/main.cpp | 58 ++++++++++++ .../write-read-write/write-read-write.pro | 4 + 12 files changed, 201 insertions(+), 13 deletions(-) create mode 100644 tests/auto/other/qprocess_and_guieventloop/qprocess_and_guieventloop.pro create mode 100644 tests/auto/other/qprocess_and_guieventloop/test.pro create mode 100644 tests/auto/other/qprocess_and_guieventloop/tst_qprocess_and_guieventloop.cpp create mode 100644 tests/auto/other/qprocess_and_guieventloop/write-read-write/main.cpp create mode 100644 tests/auto/other/qprocess_and_guieventloop/write-read-write/write-read-write.pro diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 0436aa5428..b03e96d0f6 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -1,6 +1,7 @@ /**************************************************************************** ** ** Copyright (C) 2013 Digia Plc and/or its subsidiary(-ies). +** Copyright (C) 2014 Intel Corporation ** Contact: http://www.qt-project.org/legal ** ** This file is part of the QtCore module of the Qt Toolkit. @@ -907,24 +908,33 @@ bool QProcessPrivate::tryReadFromChannel(Channel *channel) return false; qint64 available = bytesAvailableInChannel(channel); - if (available == 0) { - if (channel->notifier) - channel->notifier->setEnabled(false); - closeChannel(channel); -#if defined QPROCESS_DEBUG - qDebug("QProcessPrivate::tryReadFromChannel(%d), 0 bytes available", channel - &stdinChannel); -#endif - return false; - } + if (available == 0) + available = 1; // always try to read at least one byte char *ptr = channel->buffer.reserve(available); qint64 readBytes = readFromChannel(channel, ptr, available); + if (readBytes <= 0) + channel->buffer.chop(available); + if (readBytes == -2) { + // EWOULDBLOCK + return false; + } if (readBytes == -1) { processError = QProcess::ReadError; q->setErrorString(QProcess::tr("Error reading from process")); emit q->error(processError); #if defined QPROCESS_DEBUG qDebug("QProcessPrivate::tryReadFromChannel(%d), failed to read from the process", channel - &stdinChannel); +#endif + return false; + } + if (readBytes == 0) { + // EOF + if (channel->notifier) + channel->notifier->setEnabled(false); + closeChannel(channel); +#if defined QPROCESS_DEBUG + qDebug("QProcessPrivate::tryReadFromChannel(%d), 0 bytes available", channel - &stdinChannel); #endif return false; } diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index 3c1eeb5f91..2269740a2f 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -981,10 +981,14 @@ qint64 QProcessPrivate::readFromChannel(const Channel *channel, char *data, qint Q_ASSERT(channel->pipe[0] != INVALID_Q_PIPE); qint64 bytesRead = qt_safe_read(channel->pipe[0], data, maxlen); #if defined QPROCESS_DEBUG + int save_errno = errno; qDebug("QProcessPrivate::readFromChannel(%d, %p \"%s\", %lld) == %lld", channel - &stdinChannel, data, qt_prettyDebug(data, bytesRead, 16).constData(), maxlen, bytesRead); + errno = save_errno; #endif + if (bytesRead == -1 && errno == EWOULDBLOCK) + return -2; return bytesRead; } diff --git a/src/corelib/io/qwindowspipereader.cpp b/src/corelib/io/qwindowspipereader.cpp index 7dd2125e70..d8a3ec9b42 100644 --- a/src/corelib/io/qwindowspipereader.cpp +++ b/src/corelib/io/qwindowspipereader.cpp @@ -132,7 +132,7 @@ qint64 QWindowsPipeReader::bytesAvailable() const qint64 QWindowsPipeReader::read(char *data, qint64 maxlen) { if (pipeBroken && actualReadBufferSize == 0) - return -1; // signal EOF + return 0; // signal EOF qint64 readSoFar; // If startAsyncRead() has read data, copy it to its destination. @@ -159,6 +159,8 @@ qint64 QWindowsPipeReader::read(char *data, qint64 maxlen) emitReadyReadTimer->stop(); if (!readSequenceStarted) startAsyncRead(); + if (readSoFar == 0) + return -2; // signal EWOULDBLOCK } return readSoFar; diff --git a/src/network/socket/qlocalsocket_win.cpp b/src/network/socket/qlocalsocket_win.cpp index 96c6c0f6ea..6fef819eee 100644 --- a/src/network/socket/qlocalsocket_win.cpp +++ b/src/network/socket/qlocalsocket_win.cpp @@ -202,7 +202,17 @@ qint64 QLocalSocket::readData(char *data, qint64 maxSize) if (!maxSize) return 0; - return d->pipeReader->read(data, maxSize); + qint64 ret = d->pipeReader->read(data, maxSize); + + // QWindowsPipeReader::read() returns error codes that don't match what we need + switch (ret) { + case 0: // EOF -> transform to error + return -1; + case -2: // EWOULDBLOCK -> no error, just no bytes + return 0; + default: + return ret; + } } qint64 QLocalSocket::writeData(const char *data, qint64 maxSize) diff --git a/tests/auto/corelib/io/qprocess/test/test.pro b/tests/auto/corelib/io/qprocess/test/test.pro index 79ea53cc6b..90afeddaa0 100644 --- a/tests/auto/corelib/io/qprocess/test/test.pro +++ b/tests/auto/corelib/io/qprocess/test/test.pro @@ -2,7 +2,6 @@ CONFIG += testcase CONFIG += parallel_test CONFIG -= app_bundle debug_and_release_target QT = core testlib network -embedded: QT += gui SOURCES = ../tst_qprocess.cpp TARGET = ../tst_qprocess diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp index b67166272b..82a0f3f832 100644 --- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp +++ b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp @@ -1953,7 +1953,7 @@ void tst_QProcess::setStandardOutputFile2() process.start("testProcessEcho2/testProcessEcho2"); process.write(testdata, sizeof testdata); QPROCESS_VERIFY(process,waitForFinished()); - QVERIFY(!process.bytesAvailable()); + QCOMPARE(process.bytesAvailable(), Q_INT64_C(0)); QVERIFY(!QFileInfo(QProcess::nullDevice()).isFile()); } diff --git a/tests/auto/other/other.pro b/tests/auto/other/other.pro index e6017168d0..407237e519 100644 --- a/tests/auto/other/other.pro +++ b/tests/auto/other/other.pro @@ -23,6 +23,7 @@ SUBDIRS=\ qobjectperformance \ qobjectrace \ qsharedpointer_and_qwidget \ + qprocess_and_guieventloop \ qtokenautomaton \ windowsmobile \ @@ -71,3 +72,5 @@ wince*|!contains(QT_CONFIG, accessibility): SUBDIRS -= qaccessibility !embedded|wince*: SUBDIRS -= \ qdirectpainter +winrt: SUBDIRS -= \ + qprocess_and_guieventloop diff --git a/tests/auto/other/qprocess_and_guieventloop/qprocess_and_guieventloop.pro b/tests/auto/other/qprocess_and_guieventloop/qprocess_and_guieventloop.pro new file mode 100644 index 0000000000..e349fe5b11 --- /dev/null +++ b/tests/auto/other/qprocess_and_guieventloop/qprocess_and_guieventloop.pro @@ -0,0 +1,4 @@ +TEMPLATE = subdirs +SUBDIRS = \ + write-read-write +SUBDIRS += test.pro diff --git a/tests/auto/other/qprocess_and_guieventloop/test.pro b/tests/auto/other/qprocess_and_guieventloop/test.pro new file mode 100644 index 0000000000..54d6f194b0 --- /dev/null +++ b/tests/auto/other/qprocess_and_guieventloop/test.pro @@ -0,0 +1,5 @@ +CONFIG += testcase +CONFIG += parallel_test +QT = core gui testlib +SOURCES = tst_qprocess_and_guieventloop.cpp +TARGET = tst_qprocess_and_guieventloop diff --git a/tests/auto/other/qprocess_and_guieventloop/tst_qprocess_and_guieventloop.cpp b/tests/auto/other/qprocess_and_guieventloop/tst_qprocess_and_guieventloop.cpp new file mode 100644 index 0000000000..42153c6c80 --- /dev/null +++ b/tests/auto/other/qprocess_and_guieventloop/tst_qprocess_and_guieventloop.cpp @@ -0,0 +1,89 @@ +/**************************************************************************** +** +** Copyright (C) 2014 Intel Corporation +** Contact: http://www.qt-project.org/legal +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and Digia. For licensing terms and +** conditions see http://qt.digia.com/licensing. For further information +** use the contact form at http://qt.digia.com/contact-us. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 2.1 as published by the Free Software +** Foundation and appearing in the file LICENSE.LGPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU Lesser General Public License version 2.1 requirements +** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Digia gives you certain additional +** rights. These rights are described in the Digia Qt LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3.0 as published by the Free Software +** Foundation and appearing in the file LICENSE.GPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU General Public License version 3.0 requirements will be +** met: http://www.gnu.org/copyleft/gpl.html. +** +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#include +#include +#include + +class tst_QProcess_and_GuiEventLoop : public QObject +{ + Q_OBJECT +private slots: + void waitForAndEventLoop(); +}; + + +void tst_QProcess_and_GuiEventLoop::waitForAndEventLoop() +{ + // based on testcase provided in QTBUG-39488 + QByteArray msg = "Hello World"; + + QProcess process; + process.start("write-read-write/write-read-write", QStringList() << msg); + QVERIFY(process.waitForStarted(5000)); + QVERIFY(process.waitForReadyRead(5000)); + QCOMPARE(process.readAll().trimmed(), msg); + + // run the GUI event dispatcher once + QSignalSpy spy(&process, SIGNAL(readyRead())); + qApp->processEvents(QEventLoop::AllEvents, 100); + + // we mustn't have read anything in the event loop + QCOMPARE(spy.count(), 0); + + // ensure the process hasn't died + QVERIFY(!process.waitForFinished(250)); + + // we mustn't have read anything during waitForFinished either + QCOMPARE(spy.count(), 0); + + // release the child for the second write + process.write("\n"); + QVERIFY(process.waitForFinished(5000)); + QCOMPARE(int(process.exitStatus()), int(QProcess::NormalExit)); + QCOMPARE(process.exitCode(), 0); + QCOMPARE(spy.count(), 1); + QCOMPARE(process.readAll().trimmed(), msg); +} + +QTEST_MAIN(tst_QProcess_and_GuiEventLoop) + +#include "tst_qprocess_and_guieventloop.moc" diff --git a/tests/auto/other/qprocess_and_guieventloop/write-read-write/main.cpp b/tests/auto/other/qprocess_and_guieventloop/write-read-write/main.cpp new file mode 100644 index 0000000000..e8fc3b5cff --- /dev/null +++ b/tests/auto/other/qprocess_and_guieventloop/write-read-write/main.cpp @@ -0,0 +1,58 @@ +/**************************************************************************** +** +** Copyright (C) 2014 Intel Corporation +** Contact: http://www.qt-project.org/legal +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and Digia. For licensing terms and +** conditions see http://qt.digia.com/licensing. For further information +** use the contact form at http://qt.digia.com/contact-us. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 2.1 as published by the Free Software +** Foundation and appearing in the file LICENSE.LGPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU Lesser General Public License version 2.1 requirements +** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Digia gives you certain additional +** rights. These rights are described in the Digia Qt LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3.0 as published by the Free Software +** Foundation and appearing in the file LICENSE.GPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU General Public License version 3.0 requirements will be +** met: http://www.gnu.org/copyleft/gpl.html. +** +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#include + +int main(int argc, char **argv) +{ + const char *msg = argv[1]; + char buf[2]; + + puts(msg); + fflush(stdout); + + // wait for a newline + fgets(buf, sizeof buf, stdin); + + puts(msg); + fflush(stdout); + return 0; +} diff --git a/tests/auto/other/qprocess_and_guieventloop/write-read-write/write-read-write.pro b/tests/auto/other/qprocess_and_guieventloop/write-read-write/write-read-write.pro new file mode 100644 index 0000000000..e236e05c7d --- /dev/null +++ b/tests/auto/other/qprocess_and_guieventloop/write-read-write/write-read-write.pro @@ -0,0 +1,4 @@ +SOURCES = main.cpp +CONFIG -= qt app_bundle +CONFIG += console +DESTDIR = ./