-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/3178 crash on shutdown #3382
Fix/3178 crash on shutdown #3382
Conversation
…thread SDLCORE-534 We used aggregation in Thread class and we dont have contorol over delegate life cycle, so logically need first to delete container and then delete delegate. we are calling join() -> stop() Inside destuctor of Thread we are using delegate_->exitThreadMain(); Inside the stop() Shutdown () was removed because it is redundant. Destructor Thread will make this.
SDLCORE-679 The core crash shows evidence that something around set_delegate is crashing, on the other hand ~MessageLoopThread is deleting the thread and not assigning the thread pointer to NULL which is necessary to avoid undefined behavior
@LuxoftAKutsan In the PR description you have stated the following:
As I can see it was not removed. Remove it please or change PR description. |
threads::DeleteThread(thread_); | ||
thread_ = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LuxoftAKutsan Why did you set thread_
pointer to NULL
after deletion but did not set thread_delegate_
to NULL
after deletion? It is a pointer as well.
Potential related fix #3388 |
Thread delegates must be destroyed before threads. Otherwise, when the delegate is destroyed it will attempt to write on the thread pointer that has been deleted.
|
@@ -148,8 +148,9 @@ MessageLoopThread<Q>::MessageLoopThread(const std::string& name, | |||
template <class Q> | |||
MessageLoopThread<Q>::~MessageLoopThread() { | |||
Shutdown(); | |||
delete thread_delegate_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread delegate must be deleted before the thread itself. All threads in the project follow this pattern
@LuxoftAKutsan, can you please let us know when this is ready for re-review? |
@theresalech this work is planned. We will notify when fix with test scripts will be available. |
Agree with PM comments. Tested attached script on current develop looks like issued covered with #3388 fix. |
Fixes #3178
This PR is ready for review.
Risk
This PR makes no API changes.
Testing Plan
Script should be created.
Summary
Tasks Remaining:
CLA