Executing QProcess in QThread: memory leak
-
@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. -
@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?
-
@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?
@sitesv
I don't know how/whether your issue relates this, but in your code: you set offping.start()
, butQProcess ping;
is a local variable in yourforeach
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.... -
@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?
@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. -
@JonB @KroMignon
Agree with you, guys.
But it works...
I can reimplement to QProcess pointers of PingTester class...@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 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++; } }
@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.
-
@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++; } }
@KroMignon
I would think doing it sequentially, withwaitForFinished()
, 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 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.
-
@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.@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 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?
@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; }
-
@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?@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 ifQProcess::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 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; }
@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.