From 423e6c3635a5e0f88f38cbf0af7474bc62b26323 Mon Sep 17 00:00:00 2001 From: Alex Trotsenko Date: Mon, 4 Jan 2021 17:46:21 +0200 Subject: [PATCH] QProcess/Unix: consolidate process state tracking socket notifiers There is no reason to have the startup notifier and the death notifier be active at the same time, as the former will detect death as well. Previously, these notifiers were racing, but _q_processDied() ordered signals by calling _q_startupNotification() manually. Thus, the started()/finished() sequence was always emitted if the child process was killed anywhere. Now this ordering is simply not necessary anymore. This makes it possible to reuse the startup notifier for death notification. Change-Id: I5ebed9b5f28b19fe56c80498977a3b21be9288fd Reviewed-by: Oswald Buddenhagen --- src/corelib/io/qprocess.cpp | 18 +++---------- src/corelib/io/qprocess_p.h | 3 +-- src/corelib/io/qprocess_unix.cpp | 43 +++++++++++++++++++------------- 3 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 2e3bd9cf591..5da0df8a4ea 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -879,13 +879,9 @@ void QProcessPrivate::cleanup() delete stdinChannel.notifier; stdinChannel.notifier = nullptr; } - if (startupSocketNotifier) { - delete startupSocketNotifier; - startupSocketNotifier = nullptr; - } - if (deathNotifier) { - delete deathNotifier; - deathNotifier = nullptr; + if (stateNotifier) { + delete stateNotifier; + stateNotifier = nullptr; } closeChannel(&stdoutChannel); closeChannel(&stderrChannel); @@ -1150,14 +1146,6 @@ void QProcessPrivate::_q_processDied() drainOutputPipes(); #endif - // the process may have died before it got a chance to report that it was - // either running or stopped, so we will call _q_startupNotification() and - // give it a chance to emit started() or errorOccurred(FailedToStart). - if (processState == QProcess::Starting) { - if (!_q_startupNotification()) - return; - } - if (dying) { // at this point we know the process is dead. prevent // reentering this slot recursively by calling waitForFinished() diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h index 58d68f83cf9..a2d14d690a1 100644 --- a/src/corelib/io/qprocess_p.h +++ b/src/corelib/io/qprocess_p.h @@ -339,8 +339,7 @@ public: Q_PIPE childStartedPipe[2] = {INVALID_Q_PIPE, INVALID_Q_PIPE}; void destroyPipe(Q_PIPE pipe[2]); - QSocketNotifier *startupSocketNotifier = nullptr; - QSocketNotifier *deathNotifier = nullptr; + QSocketNotifier *stateNotifier = nullptr; int forkfd = -1; diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index c9ede9c3e9d..1ca4a0d65ec 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -378,9 +378,12 @@ void QProcessPrivate::startProcess() } if (threadData.loadRelaxed()->hasEventDispatcher()) { - startupSocketNotifier = new QSocketNotifier(childStartedPipe[0], - QSocketNotifier::Read, q); - QObject::connect(startupSocketNotifier, SIGNAL(activated(QSocketDescriptor)), + // Set up to notify about startup completion (and premature death). + // Once the process has started successfully, we reconfigure the + // notifier to watch the fork_fd for expected death. + stateNotifier = new QSocketNotifier(childStartedPipe[0], + QSocketNotifier::Read, q); + QObject::connect(stateNotifier, SIGNAL(activated(QSocketDescriptor)), q, SLOT(_q_startupNotification())); } @@ -523,12 +526,6 @@ void QProcessPrivate::startProcess() } if (stderrChannel.pipe[0] != -1) ::fcntl(stderrChannel.pipe[0], F_SETFL, ::fcntl(stderrChannel.pipe[0], F_GETFL) | O_NONBLOCK); - - if (threadData.loadRelaxed()->eventDispatcher.loadAcquire()) { - deathNotifier = new QSocketNotifier(forkfd, QSocketNotifier::Read, q); - QObject::connect(deathNotifier, SIGNAL(activated(QSocketDescriptor)), - q, SLOT(_q_processDied())); - } } struct ChildError @@ -582,13 +579,14 @@ report_errno: bool QProcessPrivate::processStarted(QString *errorMessage) { + Q_Q(QProcess); + ChildError buf; int ret = qt_safe_read(childStartedPipe[0], &buf, sizeof(buf)); - if (startupSocketNotifier) { - startupSocketNotifier->setEnabled(false); - startupSocketNotifier->deleteLater(); - startupSocketNotifier = nullptr; + if (stateNotifier) { + stateNotifier->setEnabled(false); + stateNotifier->disconnect(q); } qt_safe_close(childStartedPipe[0]); childStartedPipe[0] = -1; @@ -597,11 +595,22 @@ bool QProcessPrivate::processStarted(QString *errorMessage) qDebug("QProcessPrivate::processStarted() == %s", i <= 0 ? "true" : "false"); #endif + if (ret <= 0) { // process successfully started + if (stateNotifier) { + QObject::connect(stateNotifier, SIGNAL(activated(QSocketDescriptor)), + q, SLOT(_q_processDied())); + stateNotifier->setSocket(forkfd); + stateNotifier->setEnabled(true); + } + + return true; + } + // did we read an error message? - if (ret > 0 && errorMessage) + if (errorMessage) *errorMessage = QLatin1String(buf.function) + QLatin1String(": ") + qt_error_string(buf.code); - return ret <= 0; + return false; } qint64 QProcessPrivate::bytesAvailableInChannel(const Channel *channel) const @@ -852,8 +861,8 @@ void QProcessPrivate::waitForDeadChild() exitCode = info.status; crashed = info.code != CLD_EXITED; - delete deathNotifier; - deathNotifier = nullptr; + delete stateNotifier; + stateNotifier = nullptr; EINTR_LOOP(ret, forkfd_close(forkfd)); forkfd = -1; // Child is dead, don't try to kill it anymore