Why does QProcess readyReadStandardError/readyReadStandardOutput only get called if waitForFinished is called?

paulpv

I have created the following repo to host the code for this question:
https://github.com/NightVsKnight/QtQPromiseReadyReadStandardOutput/

I have a class Pinger that has a member QProcess m_processPing property that remains in scope throughout the duration of the app's MainWindow.
Just before I start a ping.exe -t 127.0.0.1 process, I connect QProcess::readyReadStandardError and QProcess::readyReadStandardOutput (and optionally any other QProcess signals) to lambdas.
Those lambdas (and any other QProcess signal that I connect to) get called as expected if I call m_processPing.waitForFinished(...) after starting the process.
If I do not call m_processPing.waitForFinished(...) those lambdas never get called.
Can someone educate me as to why?

Relevant snippet:

class Pinger : public QObject
{
    ...
    QProcess m_processPing;
    ...

    int startSync()
    {
        ...
        connect(&m_processPing, &QProcess::readyReadStandardError, this, [this]()
        {
            emit onStandardError(m_processPing.readAllStandardError());
        });
        connect(&m_processPing, &QProcess::readyReadStandardOutput, this, [this]()
        {
            emit onStandardOutput(m_processPing.readAllStandardOutput());
        });
        ...
        m_processPing.start("ping", QStringList() << "-t" << "127.0.0.1");
        if (m_processPing.waitForStarted(1000))
        {
            // readyReadStandardError and readyReadStandardOutput only signal if the following line is called
            m_processPing.waitForFinished(-1);
        }
        ...
        return errorCode;
    }
}

The full code can be seen at:
https://github.com/NightVsKnight/QtQPromiseReadyReadStandardOutput/blob/main/mainwindow.h

While I have y'all here, I am also wondering why m_pProcessPing->close() sometimes causes an access violation during a simple destruction:

class Pinger : public QObject
 {   
    ~Pinger()
    {
        qDebug() << "+~Pinger()";
        stopSync();
        qDebug() << "-~Pinger()";
    }

    void stopSync()
    {
        qDebug() << "+stopSync()";
        if (m_pProcessPing)
        {
#if 0
            // Why doesn't `QProcess::close()` work nicely? The app often crashes during destruction.
            qDebug() << "+m_pProcessPing->close()";
            m_pProcessPing->close();
            qDebug() << "-m_pProcessPing->close()";
#else
            // This works OK, but does not seem as nice
            qDebug() << "+m_pProcessPing->kill()";
            m_pProcessPing->kill();
            qDebug() << "-m_pProcessPing->kill()";
#endif
            m_pProcessPing = nullptr;
        }
        qDebug() << "-stopSync()";
    }
}

Thanks!

ChrisW67

QProcess, and Qt IO in general, does not run unless the program returns to the Qt event loop. Yours does not return in the normal fashion, but events are processed in the synchronous waitForFinished() call. You either use the Qt event loop in the 'normal' fashion and code that way or you use the synchronous convenience wrappers.

As for the crash, this would usually be an uninitialised or otherwise invalid pointer. The curious dance you do to get a pointer to a stack allocated QProcess, coupled with the close() call during object destruction is probably playing a part.

Perhaps you can explain the problem this seemingly overcomplicated code is trying to solve.

JonB

@paulpv said in Why does QProcess readyReadStandardError/readyReadStandardOutput only get called if waitForFinished is called?:

If I do not call m_processPing.waitForFinished(...) those lambdas never get called.

Because the ping program will be using standard C/C++ writing to stdout which is (in this case block) buffered (for efficiency). Note that you will likely find this is the case for stdout but not for stderr: if it wrote anything to stderr that would arrive immediately (because it is not buffered), but likely your ping does not have anything to write to stderr.

Replace the ping command with a tiny C program of your own which just does something like:

for (int i = 0; i < 100; i++)
{
    printf("Hello world\n");
    sleep(1);
}

When you run the from your QProcess code you will also not be seeing any readyReadStandardOutput()s as it runs along. Because the output is still buffered inside the code as shown. When you finally call waitForFinished() the program is allowed to run to end and exit, at which point it flushes and closes the stdout buffer, and all the output probably arrives at the same time in your calling program. In fact it will also flush when the total bytes output reaches some number (might be e.g. 512 bytes or 4,096 bytes), because the buffer is set to do so when it reaches some limit.

Now just insert the line fflush(stdout); immediately after the printf(). Then you should find that each line arrives immediately after output, without any need to waitForFinished().

There is nothing your Qt program can do about the fact that a program being run from QProcess is buffering its output. Only that program's code affects this.

In fact my ping under Ubuntu does not appear to buffer its output (e.g. it has a fflush()), are you perhaps running under Windows? There are also "cheat tools" like stdbuf or unbuffer under some Linuxes, again I don't know whether those are available to you.

EDIT:
@ChrisW67 said in Why does QProcess readyReadStandardError/readyReadStandardOutput only get called if waitForFinished is called?:

Yours does not return in the normal fashion, but events are processed in the synchronous waitForFinished() call.

I did not notice this in your code! In this case it is simply as @ChrisW67 says, and I need not have typed all the preceding in :) (In this case remove your waitForFinished(), allow stopSync() to exit and your code to return to the main Qt event loop, the output will now start arriving and calling your lambda, put a slot on the finished() signal instead to know when it has completed.) Still the buffering issue may be useful for you to know, for other situations.

paulpv

@ChrisW67 said in Why does QProcess readyReadStandardError/readyReadStandardOutput only get called if waitForFinished is called?:

Perhaps you can explain the problem this seemingly overcomplicated code is trying to solve.

ping is just a simple example placeholder for a more complex daemon process that I need to run while my app runs: usbipd.exe from https://github.com/cezanne/usbip-win.
(That is what I am using for now, but I am looking in to other alternatives)

I need to reliably know when that process has started or stopped (successful or error), and I want to log its stdout and stderr to qDebug and optionally a debug QTextEdit window in my app for the entirety of its lifetime.

I understand that QProcess is async in nature up until the point that any blocking method (such as waitFor* is called), so I was hoping to take advantage of that by never calling waitForFinished(-1) and just returning and letting the signals catch the ongoing output.

I interpret @ChrisW67's and @JonB 's comments that it sounds like the thread created by QtConcurrent::run needs to not exit and must continue to pump a message loop to keep reading the output, and perhaps the best way to do this is to just call waitForFinished(...).

Still, I would have thought that the 3rd parameter context used in the connect(&m_process, &QProcess::readAllStandardOutput, this, lambda) would receive m_process' output in this class' own UI event loop, even if the thread that called m_process.start(...) exits.

JonB

@paulpv
For my response you can ignore all the stuff about buffering. It was not this case.

@ChrisW67 answered correctly about the event loop needing to run for readyRead...() signals to get delivered.

All of a sudden you have introduced QtConcurrent::run() and threads. This has not been mentioned before. The situation is much more complicated if threads are involved.

waitForFinished() does not do anything you cannot do yourself using the asynchronous signals. It just runs an internal event loop and waits for finished() signal. You can look at its source code, it's only a few lines.

I need to reliably know when that process has started or stopped (successful or error), and I want to log its stdout and stderr to qDebug and optionally a debug QTextEdit window in my app for the entirety of its lifetime.

You can do all of this. I cannot say anything about your situation if you are using multiple threads and that is at issue here.

paulpv

Thanks for your feedback.
At a minimum you have helped me to simplify my code as follows, which effectively just start the process on the main UI thread.

I was using QtConcurrent because I [thought I] was seeing that m_process.waitForFinished(...) had to be called in order to read stdout/stderr, and I obviously did not want to block the main UI thread.
I must have been too new and confused, because that is apparently incorrect.

Given that wrong conclusion, I could have tried just a QThread, but I wanted to also learn how Qt does Future and Promise, thus why I went a little overboard and added QtConcurrent/ QFuture/QPromise.
I still use QFuture and QPromise elsewhere in my real app code to make async calls to the actual USBIP Daemon my app is interoperating with, but for this specific purpose of launching the Daemon (that ping is just an example placeholder for) I think that it will work fine to entirely avoid using other threads here (either QThread direct or QtConcurrent).
It even helped me reliably call m_process.close() but even better just avoid calling that by just auto call m_process's destructor (because its parent is set to the class Pinger instance).

class Pinger : public QObject
{
    Q_OBJECT
public:
    explicit Pinger(QObject *parent = nullptr)
        : QObject{parent}
        , m_processPing(this)
    { }

    int startAsync()
    {
        qDebug() << "+startAsync()";
        auto state = m_processPing.state();
        if (state != QProcess::ProcessState::NotRunning)
        {
            return 0;
        }

        ...
        connect(&m_processPing, &QProcess::readyReadStandardError, this, [this]()
        {
            emit onStandardError(m_processPing.readAllStandardError());
        });
        connect(&m_processPing, &QProcess::readyReadStandardOutput, this, [this]()
        {
            emit onStandardOutput(m_processPing.readAllStandardOutput());
        });

        m_processPing.start("ping", QStringList() << "-t" << "127.0.0.1");

        int errorCode;
        if (m_processPing.waitForStarted(100))
        {
            errorCode = 0;
        }
        else
        {
            qDebug() << "process failed to start";
            errorCode = 1;
        }
        qDebug().nospace() << "errorCode=" << errorCode;
        qDebug() << "-startAsync()";
        return errorCode;
    }

    QProcess m_processPing;
    ...
}

JonB

@paulpv
OK. Introducing threads in order to use waitFor...() was a red-herring :) I never use the waitFor...()s. I wouldn't even bother with your waitForStarted() here, it does not add anything. That just connects stateChanged() signal and waits for QProcess::Running, you don't usually need this. Do connect errorOccurred() as well as finished(), that is all you need.

One thing. The way you have written startAsync() implies you might intend to re-use m_processPing/this instance to run a process again at a future time? If so, IIRC, the connect()s will add another duplicate call to the slots (Qt does not default to unique connections, as I say IIRC). Usually you do the connect()s just once, at QProcess/your class construction time.

paulpv

@JonB Thanks! Changes made to my local code!