Fix TQThreadStorage destruction in the main thread #127

Merged
MicheleC merged 1 commits from fix/threadstorage-destroy-main into master 2 months ago
Collaborator

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.

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.
Fat-Zer added 1 commit 2 months ago
1ed4c6774e
Fix TQThreadStorage destruction in the main thread
Fat-Zer force-pushed fix/threadstorage-destroy-main from 1ed4c6774e to 97049a363f 2 months ago
MicheleC requested changes 2 months ago
MicheleC left a comment
Owner

See comments.

See comments.
private:
TQThreadInstance * d;
friend class TQThreadInstance;
friend class TQThreadStorageData;
Owner

Not required, see this comment

Not required, see [this comment](https://mirror.git.trinitydesktop.org/gitea/TDE/tqt3/pulls/127/files#issuecomment-47352)
MicheleC marked this conversation as resolved
{
// avoid warning from TQThread
d->running = false;
// do some cleanup
Owner

Suggest we make the comment a bit more explicit, something like Cleanup thread storage for the GUI thread before exiting or something like that.

Suggest we make the comment a bit more explicit, something like `Cleanup thread storage for the GUI thread before exiting` or something like that.
Poster
Collaborator

done

done
Fat-Zer marked this conversation as resolved
static void finish( TQThreadInstance * );
#endif // Q_OS_WIN32
static void finishMain( TQThreadInstance *d );
Owner

Suggest renaming into finishMainThread or finishGUIThread

Suggest renaming into `finishMainThread` or `finishGUIThread`
Poster
Collaborator

done

done
Fat-Zer marked this conversation as resolved
TQThreadStorageData::~TQThreadStorageData()
{
// Main thread won't be able to cleanup its storage upon exit, so we will do it for them
Owner

This does not seem to be required at all. The call to finishMain() in qApplication#L564 will invoke the TQThreadStorageData::finish() function which will clean up all the objects stored for this thread.

This does not seem to be required at all. The call to `finishMain()` in `qApplication#L564` will invoke the `TQThreadStorageData::finish()` function which will clean up all the objects stored for this thread.
Poster
Collaborator

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 the TQThreadStorageData::finish()) or the TQThreadStorageData::~TQThreadStorageData(). In the later case TQThreadStorageData::finish() will either die trying to print the warning or won't be able to destroy the object because thread_storage_usage[i].func will be already nullified.

I'll add the comment to source to be more explicit...

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 the `TQThreadStorageData::finish()`) or the `TQThreadStorageData::~TQThreadStorageData()`. In the later case `TQThreadStorageData::finish()` will either die trying to print [the warning](https://mirror.git.trinitydesktop.org/gitea/TDE/tqt3/src/commit/97049a363f2c2b34a3f84b0ba9bdbf8e699f2fa5/src/tools/qthreadstorage_unix.cpp#L166) or won't be able to destroy the object because `thread_storage_usage[i].func` will be already nullified. I'll add the comment to source to be more explicit...
Poster
Collaborator

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...

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...
Poster
Collaborator

Ok, I've done some small modifications to how thread object is queried and now TQApplication::guiThread() should properly return 0 than the thread object is destruct.

Ok, I've done some small modifications to how thread object is queried and now `TQApplication::guiThread()` should properly return `0` than the thread object is destruct.
Owner

Usually the TQThreadStorage<> objects are static the same as QApplication and generally speaking it's impossible to predict which will be called first...

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

> Usually the TQThreadStorage<> objects are static the same as QApplication and generally speaking it's impossible to predict which will be called first... 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
Poster
Collaborator

Yep, no hurry for this one...

PS: I wouldn't think of it either unless I've run directly into it.

Yep, no hurry for this one... PS: I wouldn't think of it either unless I've run directly into it.
Owner

Solution looks good. Few types to correct in the comment:

  1. Gui Thread has staic --> Gui thread has static
  2. it's storage --> its storage
  3. destructor bellow --> destructor below
Solution looks good. Few types to correct in the comment: 1. `Gui Thread has staic` --> `Gui thread has static` 2. `it's storage` --> `its storage` 3. `destructor bellow` --> `destructor below`
Poster
Collaborator

Solution looks good. Few types to correct in the comment:

  1. Gui Thread has staic --> Gui thread has static
  2. it's storage --> its storage
  3. destructor bellow --> destructor below

fixed

> Solution looks good. Few types to correct in the comment: > 1. `Gui Thread has staic` --> `Gui thread has static` > 2. `it's storage` --> `its storage` > 3. `destructor bellow` --> `destructor below` fixed
MicheleC marked this conversation as resolved
Fat-Zer force-pushed fix/threadstorage-destroy-main from 97049a363f to 40fd140261 2 months ago
MicheleC requested changes 2 months ago
MicheleC left a comment
Owner

A few points to adjust, but overall I am happy with the solution

A few points to adjust, but overall I am happy with the solution
public:
inline TQCoreApplicationThread()
{
tqt_gui_thread_self = this;
Owner

Suggest we check whether tqt_gui_thread_self is null or not and raised a fatal error if not, like in the run() method. TQCoreApplicationThread should be unique and the problem should never happen, but good to enforce it from code too.

Suggest we check whether `tqt_gui_thread_self` is null or not and raised a fatal error if not, like in the `run()` method. `TQCoreApplicationThread` should be unique and the problem should never happen, but good to enforce it from code too.
Poster
Collaborator

unlike run() it can't be run from outside this file, so tqWarning() under QT_CHECK_STATE would fit better...

unlike `run()` it can't be run from outside this file, so `tqWarning()` under `QT_CHECK_STATE` would fit better...
Owner

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 :-)

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 :-)
MicheleC marked this conversation as resolved
static TQCoreApplicationThread* tqt_gui_thread_self;
};
TQCoreApplicationThread* TQCoreApplicationThread::tqt_gui_thread_self = 0;
Owner

Use nullptr like in the destructor? Just for consistency

Use `nullptr` like in the destructor? Just for consistency
Poster
Collaborator

fixed

fixed
Fat-Zer marked this conversation as resolved
Fat-Zer force-pushed fix/threadstorage-destroy-main from 40fd140261 to 1d9ce4d25a 2 months ago
Fat-Zer requested review from MicheleC 2 months ago
MicheleC approved these changes 2 months ago
MicheleC left a comment
Owner

Looks good

Looks good
MicheleC added this to the R14.1.2 release milestone 2 months ago
MicheleC merged commit b1e6f38464 into master 2 months ago
MicheleC deleted branch fix/threadstorage-destroy-main 2 months ago

Reviewers

MicheleC approved these changes 2 months ago
The pull request has been merged as b1e6f38464.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tqt3#127
Loading…
There is no content yet.