From 6a6d12ea4b90d24839f13b92237dbc8cce5eaf9d Mon Sep 17 00:00:00 2001 From: Alex Trotsenko Date: Tue, 5 Jan 2021 18:59:40 +0200 Subject: Split QProcessPrivate::_q_processDied() The completion of the child process can take place asynchronously or in one of the waitFor...() functions. In both cases, we used the same handler (_q_processDied()), which caused several problems: a. technically, waitForReadyRead() should have taken into account the result of the calls to _q_canRead...() slots inside the _q_processDied() function: - the user calls waitForReadyRead(); - forkfd descriptor becomes signaled, while a grandchild process is still alive; - as readyRead() signal has not been emitted, _q_processDied() is called; - the grandchild process writes to stdout pipe; - now data arrives, and _q_processDied() will collect it, but won't report it. b. we had a bug with recursions on Unix: - death notification comes asynchronously; - waitForDeadChild() closes forkfd; - _q_canRead...() emits readyRead(); - a slot connected to readyRead() calls waitForFinished(); - waitForFinished() hangs (forkfd == -1). c. for blocking functions, drainOutputPipes() was called twice on Windows. By introducing a new processFinished() function, we leave the read operations in the _q_processDied() slot, while the process completion code is guaranteed to run only once. Change-Id: I5f9d09bc68a058169de4d9e490b48fc0b35e94cd Reviewed-by: Oswald Buddenhagen --- src/corelib/io/qprocess.cpp | 67 +++++++++++++++++++++------------------- src/corelib/io/qprocess_p.h | 2 +- src/corelib/io/qprocess_unix.cpp | 11 ++++--- src/corelib/io/qprocess_win.cpp | 8 +++-- 4 files changed, 48 insertions(+), 40 deletions(-) (limited to 'src/corelib/io') diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 5da0df8a4e..48db7cdd54 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -1133,56 +1133,62 @@ bool QProcessPrivate::_q_canWrite() */ void QProcessPrivate::_q_processDied() { - Q_Q(QProcess); #if defined QPROCESS_DEBUG qDebug("QProcessPrivate::_q_processDied()"); #endif + + // in case there is data in the pipeline and this slot by chance + // got called before the read notifications, call these functions + // so the data is made available before we announce death. +#ifdef Q_OS_WIN + drainOutputPipes(); +#endif + _q_canReadStandardOutput(); + _q_canReadStandardError(); + + // Slots connected to signals emitted by the functions called above + // might call waitFor*(), which would synchronously reap the process. + // So check the state to avoid trying to reap a second time. + if (processState != QProcess::NotRunning) + processFinished(); +} + +/*! + \internal +*/ +void QProcessPrivate::processFinished() +{ + Q_Q(QProcess); +#if defined QPROCESS_DEBUG + qDebug("QProcessPrivate::processFinished()"); +#endif + #ifdef Q_OS_UNIX waitForDeadChild(); #endif #ifdef Q_OS_WIN if (processFinishedNotifier) processFinishedNotifier->setEnabled(false); - drainOutputPipes(); #endif - - if (dying) { - // at this point we know the process is dead. prevent - // reentering this slot recursively by calling waitForFinished() - // or opening a dialog inside slots connected to the readyRead - // signals emitted below. - return; - } - dying = true; - - // in case there is data in the pipe line and this slot by chance - // got called before the read notifications, call these two slots - // so the data is made available before the process dies. - _q_canReadStandardOutput(); - _q_canReadStandardError(); - findExitCode(); + cleanup(); + if (crashed) { exitStatus = QProcess::CrashExit; setErrorAndEmit(QProcess::Crashed); } - bool wasRunning = (processState == QProcess::Running); + // we received EOF now: + emit q->readChannelFinished(); + // in the future: + //emit q->standardOutputClosed(); + //emit q->standardErrorClosed(); - cleanup(); - - if (wasRunning) { - // we received EOF now: - emit q->readChannelFinished(); - // in the future: - //emit q->standardOutputClosed(); - //emit q->standardErrorClosed(); + emit q->finished(exitCode, exitStatus); - emit q->finished(exitCode, exitStatus); - } #if defined QPROCESS_DEBUG - qDebug("QProcessPrivate::_q_processDied() process is dead"); + qDebug("QProcessPrivate::processFinished(): process is dead"); #endif } @@ -2253,7 +2259,6 @@ void QProcessPrivate::start(QIODevice::OpenMode mode) stderrChannel.closed = false; exitCode = 0; - dying = false; exitStatus = QProcess::NormalExit; processError = QProcess::UnknownError; errorString.clear(); diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h index a2d14d690a..4364d99c9b 100644 --- a/src/corelib/io/qprocess_p.h +++ b/src/corelib/io/qprocess_p.h @@ -309,7 +309,6 @@ public: qint64 pid = 0; #endif - bool dying = false; bool emittedReadyRead = false; bool emittedBytesWritten = false; @@ -354,6 +353,7 @@ public: void execChild(const char *workingDirectory, char **argv, char **envp); #endif bool processStarted(QString *errorMessage = nullptr); + void processFinished(); void terminateProcess(); void killProcess(); void findExitCode(); diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index 1ca4a0d65e..17a1425d3f 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -754,8 +754,10 @@ bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline) if (processState == QProcess::NotRunning) return false; + // We do this after checking the pipes, so we cannot reach it as long + // as there is any data left to be read from an already dead process. if (qt_pollfd_check(poller.forkfd(), POLLIN)) { - _q_processDied(); + processFinished(); return false; } } @@ -796,7 +798,7 @@ bool QProcessPrivate::waitForBytesWritten(const QDeadlineTimer &deadline) return false; if (qt_pollfd_check(poller.forkfd(), POLLIN)) { - _q_processDied(); + processFinished(); return false; } } @@ -837,7 +839,7 @@ bool QProcessPrivate::waitForFinished(const QDeadlineTimer &deadline) return true; if (qt_pollfd_check(poller.forkfd(), POLLIN)) { - _q_processDied(); + processFinished(); return true; } } @@ -850,8 +852,7 @@ void QProcessPrivate::findExitCode() void QProcessPrivate::waitForDeadChild() { - if (forkfd == -1) - return; // child has already been reaped + Q_ASSERT(forkfd != -1); // read the process information from our fd forkfd_info info; diff --git a/src/corelib/io/qprocess_win.cpp b/src/corelib/io/qprocess_win.cpp index 6ab854de16..2a5b8fd4fd 100644 --- a/src/corelib/io/qprocess_win.cpp +++ b/src/corelib/io/qprocess_win.cpp @@ -685,7 +685,7 @@ bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline) if (WaitForSingleObjectEx(pid->hProcess, 0, false) == WAIT_OBJECT_0) { bool readyReadEmitted = drainOutputPipes(); if (pid) - _q_processDied(); + processFinished(); return readyReadEmitted; } @@ -743,7 +743,9 @@ bool QProcessPrivate::waitForBytesWritten(const QDeadlineTimer &deadline) // Wait for the process to signal any change in its state, // such as incoming data, or if the process died. if (WaitForSingleObjectEx(pid->hProcess, 0, false) == WAIT_OBJECT_0) { - _q_processDied(); + drainOutputPipes(); + if (pid) + processFinished(); return false; } @@ -782,7 +784,7 @@ bool QProcessPrivate::waitForFinished(const QDeadlineTimer &deadline) if (WaitForSingleObject(pid->hProcess, timer.nextSleepTime()) == WAIT_OBJECT_0) { drainOutputPipes(); if (pid) - _q_processDied(); + processFinished(); return true; } -- cgit v1.2.3