Fix TQThreadStorage destruction in the main thread #127
Merged
MicheleC
merged 1 commits from fix/threadstorage-destroy-main
into master
2 months ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'fix/threadstorage-destroy-main'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Before that the allocations of TQThreadStorage objects from the main thread were never destroyed and memory associated with them were never freed. The second one isn't a huge problem as at that point program is terminating anyway (but it still makes valgrind complain). The first one is the bigger issue as destructors might contain some essential external cleanups like removing temporary files.
1ed4c6774e
to97049a363f
2 months agoSee comments.
private:
TQThreadInstance * d;
friend class TQThreadInstance;
friend class TQThreadStorageData;
Not required, see this comment
{
// avoid warning from TQThread
d->running = false;
// do some cleanup
Suggest we make the comment a bit more explicit, something like
Cleanup thread storage for the GUI thread before exiting
or something like that.done
static void finish( TQThreadInstance * );
#endif // Q_OS_WIN32
static void finishMain( TQThreadInstance *d );
Suggest renaming into
finishMainThread
orfinishGUIThread
done
TQThreadStorageData::~TQThreadStorageData()
{
// Main thread won't be able to cleanup its storage upon exit, so we will do it for them
This does not seem to be required at all. The call to
finishMain()
inqApplication#L564
will invoke theTQThreadStorageData::finish()
function which will clean up all the objects stored for this thread.Usually the
TQThreadStorage<>
objects are static the same as QApplication and generally speaking it's impossible to predict which will be called first the destructor for QApplication (and theTQThreadStorageData::finish()
) or theTQThreadStorageData::~TQThreadStorageData()
. In the later caseTQThreadStorageData::finish()
will either die trying to print the warning or won't be able to destroy the object becausethread_storage_usage[i].func
will be already nullified.I'll add the comment to source to be more explicit...
Hmm.. it will actually work only if the
TQThreadStorage
gets destroyed before the TQApplication, otherwise it will always result in a segfault... It should actually always be the case, unless tqt is statically link... I need to think about it...Ok, I've done some small modifications to how thread object is queried and now
TQApplication::guiThread()
should properly return0
than the thread object is destruct.That's a very good point, I didn't think of that. Will review this PR again this week, I first have to review a few things for Philippe, he has been waiting for a week already
Yep, no hurry for this one...
PS: I wouldn't think of it either unless I've run directly into it.
Solution looks good. Few types to correct in the comment:
Gui Thread has staic
-->Gui thread has static
it's storage
-->its storage
destructor bellow
-->destructor below
fixed
97049a363f
to40fd140261
2 months agoA few points to adjust, but overall I am happy with the solution
public:
inline TQCoreApplicationThread()
{
tqt_gui_thread_self = this;
Suggest we check whether
tqt_gui_thread_self
is null or not and raised a fatal error if not, like in therun()
method.TQCoreApplicationThread
should be unique and the problem should never happen, but good to enforce it from code too.unlike
run()
it can't be run from outside this file, sotqWarning()
underQT_CHECK_STATE
would fit better...I was thinking more about the case someone tries to instantiate two
TQCoreApplicationThread
objects by mistake. Should never happen of course, but you never know. In such case the first object thread storage would never get properly cleared (yeah, I know, properly overthinking the whole thing :-) ).EDIT
It's fine, we can keep it as it is, since the class is local to the file and we know (or should know) what we are doing :-)
static TQCoreApplicationThread* tqt_gui_thread_self;
};
TQCoreApplicationThread* TQCoreApplicationThread::tqt_gui_thread_self = 0;
Use
nullptr
like in the destructor? Just for consistencyfixed
40fd140261
to1d9ce4d25a
2 months agoLooks good
b1e6f38464
into master 2 months agoReviewers
b1e6f38464
.