Executing QProcess in QThread: memory leak

KroMignon

@sitesv said in Executing QProcess in QThread: memory leak:

Is it possible to make a checking ping of a list of addresses for each timer interval? I haven't used a lambda yet in my practice ((.

Yes, it is possible.
I have give you a basic implementation example.
You just have to complete it.

If you want to learn lambda, search on internet, there are many explanations. It is not so hard to understand.
My favorite is https://blog.feabhas.com/2014/03/demystifying-c-lambdas/

sitesv

@KroMignon said in Executing QProcess in QThread: memory leak:

Yes, it is possible.

I have tried to do something like this. But is execute too slow.

QStringList ip_list = {"192.168.0.1", "192.168.0.2"};
    int success_count = 0;
    foreach(auto ip, ip_list)
    {
        QProcess ping;
        QEventLoop l;
        ping.setProcessChannelMode(QProcess::MergedChannels);
        connect(&ping,
                QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
                [&l, &ping, &success_count]() {
                    QString output(ping.readAll());
                    if(output.contains("ttl",Qt::CaseInsensitive)) success_count++;
                    l.exit();
                });
        ping.start("/bin/ping", QStringList() << ip << "-c" << "1");
        l.exec();
    }
    if(success_count == ip_list.count())
       emit setStatus(true);
    else
       emit setStatus(false);
    m_timer->start();

jsulm

This post is deleted!

sitesv

UPD:
I found the cause of this situation. Ip's are not available. How to set a timeout for the process execution in this case?

jsulm

@sitesv Why do you actually use a local event loop? You can implement this functionality without blocking your app with a local event loop.
Also, you terminate that event loop already in the first lambda call (when the first process finishes)...

sitesv

@jsulm Local EventLoop was recommended before. Could you please advise something?

jsulm

@sitesv You know how many processes you started. So, count how many processes already finished (inside the lambda). And as soon as all processes finished you can check success_count and emit the signal. All this can be done inside lambda.

KroMignon

@sitesv said in Executing QProcess in QThread: memory leak:

How to set a timeout for the process execution in this case?

Have you tried to look at ping parameters (ping --help)

And I would only wait once, for all pings to be done:

QStringList ip_list = {"192.168.0.1", "192.168.0.2"};
int success_count = 0;
int pingsToDo = ip_list.count();
QEventLoop l;
foreach(auto ip, ip_list)
{
    QProcess ping;
    ping.setProcessChannelMode(QProcess::MergedChannels);
    connect(&ping,
            QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
            [&l, &ping, &success_count, &pingsToDo]() {
                --pingsToDo;
                QString output(ping.readAll());
                if(output.contains("ttl",Qt::CaseInsensitive)) success_count++;
                if(!pingsToDo)
                    l.exit();
            });
    ping.start("/bin/ping", QStringList() << ip << "-c" << "1");
}

// wait all pings done
l.exec();
if(success_count == ip_list.count())
   emit setStatus(true);
else
   emit setStatus(false);
m_timer->start();

sitesv

@KroMignon said in Executing QProcess in QThread: memory leak:

And I would only wait once, for all pings to be done:

Wow! Very nice!! Thank you!

@KroMignon
UPD: app is freezing on line l.exec() althought l.exit() is done....

sitesv

Guys, how to resolve this?

KroMignon

@sitesv said in Executing QProcess in QThread: memory leak:

Guys, how to resolve this?

Hmm, I think it is a tricky issue. I would start with forcing QueuedConnection, to avoid threading issues:

connect(&ping,
            QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
            [&l, &ping, &success_count, &pingsToDo]() {
                ...
            }, Qt::QueuedConnection);

sitesv

@KroMignon said in Executing QProcess in QThread: memory leak:

connect(&ping,
QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
&l, &ping, &success_count, &pingsToDo {
...
}, Qt::QueuedConnection);

'connect' failed again

KroMignon

@sitesv ~~Can you show the code you have written~~

Sorry my fault, should be:

connect(&ping,
        QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
        &l, // receiver!
        [&l, &ping, &success_count, &pingsToDo]() {
            ...
        }, Qt::QueuedConnection);

sitesv

@KroMignon

QStringList ip_list = {"192.168.0.1", "192.168.0.2"};
    int success_count = 0;
    int pingsToDo = ip_list.count();
    QEventLoop l; // used for passive wait until process finished
    foreach(auto ip, ip_list)
    {
        QProcess ping;
        ping.setProcessChannelMode(QProcess::MergedChannels);
        connect(&ping,
                QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished), &l,
                [&l, &ping, &success_count, &pingsToDo]() {
                    --pingsToDo;
                    QString output(ping.readAll()); // <<<<<<<<<<<<<<<< SIGSEGV Segmentation fault
                    if(output.contains("ttl",Qt::CaseInsensitive)){
                        success_count++;
                    }
                    if(!pingsToDo){
                        l.exit();
                    }
                }, Qt::QueuedConnection);
        ping.start("/bin/ping", QStringList() << ip << "-c" << "1");
    }
    l.exec();
    if(success_count == ip_list.count()) emit setLedStatus(0, 0, true);
    else                                 emit setLedStatus(0, 0, false);

    m_timer->start();

There is SIGSEGV Segmentation fault on ping.readAll string. :((

KroMignon

@sitesv Sorry but I was a little bit confused when I suggest you this code.
This cannot work, because QProcess is killed/destroyed at end of the for loop!
Should be:

QStringList ip_list = {"192.168.0.1", "192.168.0.2"};
int success_count = 0;
int pingsToDo = ip_list.count();
QEventLoop l;
foreach(auto ip, ip_list)
{
    auto ping = new QProcess();
    ping->setProcessChannelMode(QProcess::MergedChannels);
    connect(ping,
            QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
            [&l, ping, &success_count, &pingsToDo]() {
                --pingsToDo;
                QString output(ping->readAll());
                if(output.contains("ttl",Qt::CaseInsensitive))
                    success_count++;
                // free memory
                ping->deleteLater();
                //  exit event loop after all pings done
                if(!pingsToDo)
                    l.exit();
            });
    ping->start("/bin/ping", QStringList() << ip << "-c" << "1");
}

// wait all pings done
l.exec();

JonB

@sitesv , @KroMignon
I will say one thing.

You are writing code which will call QEventLoop::exec(), blocking whatever calls it. It relies on hitting the QEventLoop::exit() statement, which you only have in response to the QProcess::finished signal. If for whatever reason that does not get hit, your event loop will never be exited.

I would not write production (or even development) code like this, and certainly not for distribution. It is asking for an "unseen hang" to occur, one day. I would at minimum hook onto QProcess::errorOccurred, maybe stateChanged() too. And I would put in some sort of timer/timeout, so that if something goes badly wrong you get out of the blocking loop (with perhaps an error flag) instead of waiting for Hell to freeze over....

KroMignon

@JonB said in Executing QProcess in QThread: memory leak:

I would not write production (or even development) code like this, and certainly not for distribution. It is asking for an "unseen hang" to occur, one day. I would at minimum hook onto QProcess::errorOccurred, maybe stateChanged() too. And I would put in some sort of timer/timeout, so that if something goes badly wrong you get out of the blocking loop (with perhaps an error flag) instead of waiting for Hell to freeze over....

Yes, this is a good advice.

sitesv

@JonB said in Executing QProcess in QThread: memory leak:

You are writing code which will call QEventLoop::exec(), blocking whatever calls it. It relies on hitting the QEventLoop::exit() statement, which you only have in response to the QProcess::finished signal. If for whatever reason that does not get hit, your event loop will never be exited.

Maybe my first variant was good (QThread + QProcess)?

if(!myProcess) myProcess = new QProcess(this);
myProcess->start(exe_path, arguments);
myProcess->waitForFinished(500);
output = myProcess->readAll();
output_str = codec->toUnicode(output);
output_strlst = output_str.split("\r\n");
myProcess->close();
...

There is QProcess "waitForFinished" with timeout.
The app wasn't freezing with this approach.

KroMignon

@sitesv said in Executing QProcess in QThread: memory leak:

There is QProcess "waitForFinished" with timeout.
The app wasn't freezing with this approach.

Why not, but you have to wait ping finished before starting next.
If it is what you want, then go with it.

jsulm

@sitesv I'm still wondering why you think you need a local event loop...