Executing QProcess in QThread: memory leak

jsulm

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

sitesv

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

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

Hi! I have done variant as you recommend. It's working!

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

I made another experiment:

Created a QThread
In QThread::run() method::

made a QTimer* object,
set him as "one shot kind",
ran QTimer, and in final
* ran QThread::exec() (for run local EventLoop)...

There is timer->start() in timeout slot, It executed, but QTimer doesn't start again.

Any ideas?

JonB

@sitesv
I don't know how/whether your issue relates this, but in your code: you set off ping.start(), but QProcess ping; is a local variable in your foreach loop and so immediately goes out of scope (not to mention, you also re-use the same local variable for each time round the loop, overwriting/destroying the previous one). Nothing should work (it might actually "crash"), I don't understand how you say it does.

On top of all of this: I think we've said it already here above, but goodness only knows why you are using a thread, with all the complications that involves? If you want to run a QProcess and the main thread to know when it's finished, it's asynchronous anyway, it would be a whole lot simpler not to have any thread. I think I said this earlier, but up to you.

And finally, while I'm on a roll: I think you are just using a /bin/ping in order to read the textual output, parse it, and see whether something is there/alive. In which case I'd be tempted to just write it myself in Qt instead of running some external command, I would have thought it's only a few lines of code. Your "parsing" of the output is beyond hokey, not even sure what you think it tells you....

KroMignon

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

Hi! I have done variant as you recommend. It's working!

I am not sure this is really working!
I think you have to (re)learn C++ object life cycle.
You create a QProcess local instance in the for loop, this object will be destroyed at loop end.

sitesv

@JonB @KroMignon
Agree with you, guys.
But it works...
I can reimplement to QProcess pointers of PingTester class...

KroMignon

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

But it works...

It don't, "it seems to work" would be the correct answer ;)

If you try to ping a not valid/accessible IP address, I am pretty sure it will not work.

If you want to do it sequentially, you have to be consistent in your choice:

foreach(auto ip, ip_list)
{
    QProcess ping;
    ping.setProcessChannelMode(QProcess::MergedChannels);
    ping.start("/bin/ping", QStringList() << ip << "-c" << "1" << "-w" << "1");
    ping.waitForFinished(5000); // wait up to 5 seconds
    QString output(ping.readAll());
    if(output.contains("ttl",Qt::CaseInsensitive)){
        success_count++;
    }
}

sitesv

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

If you try to ping a not valid/accessible IP address, I am pretty sure it will not work.

There is "-w 1" key. It helps with unaccessible IP.

JonB

@KroMignon
I would think doing it sequentially, with waitForFinished(), for his multiple IP addresses would be a poor way to do things. He has a number of IP addresses to check, these should be done in parallel....

KroMignon

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

There is "-w 1" key. It helps with unaccessible IP.

AFAIK "-w 1" will wait up to 1 second.
I am sure you will got QProcess warnings about killing a process which is still running.

sitesv

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

AFAIK "-w 1" will wait up to 1 second.

...and QProcess will emit a "finished" signal. Why you are writing about "killing" process?

KroMignon

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

...and QProcess will emit a "finished" signal. Why you are writing about "killing" process?

This is the way I would implement multiple pings in parallel:

int PingTester::doPing(const QStringList &ip_list)
{
    // to avoid issues ;)
    if(ip_list.isEmpty())
        return 0;

    int success_count = 0;
    int pingsToDo = ip_list.count();
    QEventLoop l;
    QTimer timer;
    foreach(auto ip, ip_list)
    {
        auto ping = new QProcess();
        ping->setProcessChannelMode(QProcess::MergedChannels);
        connect(timer, &QTimer::timeout, ping, &QProcess::kill);
        connect(ping, &QProcess::stateChanged,
                [&l, ping, &success_count, &pingsToDo](QProcess::ProcessState newState) {
                    if(newState != QProcess::NotRunning)
                        return;
                    --pingsToDo;
                    QString output(ping->readAll());
                    if(output.contains("ttl",Qt::CaseInsensitive))
                        success_count++;
                    // free memory
                    ping->deleteLater();
                    // exit loop when done.
                    if(!pingsToDo)
                        l.exit();
                });
        ping->start("/bin/ping", QStringList() << ip << "-c" << "1" << "-w" << "1");
    }
    timer.start(1500); // kill process after 1.5 seconds if still running
    // wait all pings done
    l.exec();
    return success_count;
}

sitesv

@jsulm There is a local QEventLoop...
@KroMignon I really don't understand why you are checking state changing of QProcess...
QProcess "Finished" method is not suitable?
And why kill a process?

KroMignon

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

QProcess "Finished" method is not suitable?

I found it easier to use QProcess::stateChanged(), because there is no overload of this slot, and I am not sure if QProcess::finished(int, QProcess::ExitStatus) is triggered if process is killed.

And why kill a process?

This is a security, when calling an external application, many things can happen: execution right issues, application not exist, network issues and so on.
99.9% of time it will not be useful, but it will ensure QEventLoop will exit at the end!

sitesv

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

This is the way I would implement multiple pings in parallel:

This code doesn't work. Only one iteration.