Do not crash on GUI shutdown

[ntfshard]

Seems current code for worker threads termination is too brutal which leads to crash on termination:

QThread::start: Thread termination error: No such process
Segmentation fault (core dumped)

Seems better to use quit() and wait(), like in an example: https://doc.qt.io/qt-6/qthread.html#details

tested: Ubuntu Linux 20

[ntfshard]

bt from gdb:

Thread 1 "cppcheck-gui" received signal SIGSEGV, Segmentation fault.
0x00007ffff6ae608e in __pthread_cancel (th=140736860096256) at pthread_cancel.c:33
33	pthread_cancel.c: No such file or directory.
(gdb) bt
#0  0x00007ffff6ae608e in __pthread_cancel (th=140736860096256) at pthread_cancel.c:33
#1  0x00007ffff6bb8601 in QThread::terminate() () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x0000555555ac009a in ThreadHandler::removeThreads() (this=0x555555f5ce40) at threadhandler.cpp:155
#3  0x0000555555abf79a in ThreadHandler::~ThreadHandler() (this=0x555555f5ce40, __in_chrg=<optimized out>) at threadhandler.cpp:50
#4  0x0000555555abf84e in ThreadHandler::~ThreadHandler() (this=0x555555f5ce40, __in_chrg=<optimized out>) at threadhandler.cpp:51
#5  0x00007ffff6da9eee in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007ffff7997c29 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x0000555555a4f067 in MainWindow::~MainWindow() (this=0x7fffffffdc90, __in_chrg=<optimized out>) at mainwindow.cpp:310
#8  0x0000555555a4a95a in main(int, char**) (argc=1, argv=0x7fffffffdeb8) at main.cpp:85

[firewave]

The wait() was definitely missing.

The quit() part seems tricky as it tells the "event loop" to exit. That seems indicates that it needs to hit some check in the loop to actually exit in case of a hang or a long-running operation the thread would probably still finish up what it is doing currently and thus not be "stopped" at all.
Whereas terminate() tells the operating system to terminate the thread. So anything running should be aborted "immediately".

An improvement would be to stop and wait the threads in parallel. But that is out of scope.

[ntfshard]

An improvement would be to stop and wait the threads in parallel. But that is out of scope.

Or much better to use existing libraries

[firewave]

Or much better to use existing libraries

What? Qt is an "existing library".

The actual problem is that you are trying to terminate a thread which has already finished. It is a race condition. So isFinished() should be called before terminate(). But by design that would still leave room for the data race - especially with the current sequential processing of the threads. That would be further improved by making the stopping parallel.

It is strange though that your initial error says QtThread::start which might indicate it was marked for termination while still being started. So we might need to do this totally different with enhanced logic and leverage the started() and finished() callbacks.

quit() would actually change the behavior and only interrupt the analysis at the next possible point (which might never come) or not forcefully stop it right now. wait() would already make this action slightly slower as before.

So for now I would go with isFinished() and terminate() for now and file a ticket about reviewing this further.

We should also file a bug report with Qt as it only mentions that it might leads to inconsistent states but not outright crashes.

[ntfshard]

Or much better to use existing libraries

What? Qt is an "existing library".

I told about 3rd party thread pools and parallel execution, like QThreadPool, or TBB library, or execution policy in STL.

I see, method which runs event loop for thread is overridden and quit event goes to nothing. And CheckThread::mState changes not atomically in method stop().

[danmar]

Whereas terminate() tells the operating system to terminate the thread. So anything running should be aborted "immediately".

I have a doubt about that. That means all memory and resources owned by the thread will leak right?

[ntfshard]

Whereas terminate() tells the operating system to terminate the thread. So anything running should be aborted "immediately".

I have a doubt about that. That means all memory and resources owned by the thread will leak right?

Seems so. I believe this code should be refactored, as I mentioned before. But at least I just want to avoid annoying crashes on exit and tons of coredump files. Will rename PR

[danmar]

I would say this PR is an improvement so I think it is merge-able.

But I wonder if it still works as we want. Could you please tell me what happens..

I assume the Settings::terminate() is called first to allow the threads to quit gracefully. Is there some delay after that before the thread->terminate() is called? I have the feeling that it would be a good idea to wait something like 1-2 seconds for the graceful shutdown.

[firewave]

The changes improve things.

See #5269 (comment) for some notes on the overall issue.

[danmar]

There is now a merge conflict. If you resolve that I think we can merge.