Remove Thread::kill() and related unittest #10317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pyrollo
reviewed
Aug 29, 2020
|
if we use only kill on our test, it's just a useless method. Drop it, less maintenance to do on dead called code :) |
|
It is still called by Thread destructor. I guess the code could be moved there then. |
|
i agree with you @pyrollo |
|
I will look to merge the kill() method inside the destructor
|
The Thread kill() method isn't used outside testThreadKill unit test and Thread destructor. It could be make private. The testThreadKill test should be removed: it tests the kill() feature in a way minetest doesn't use it (kill and restart a thread), specially because some pthreads implementations on BSD will not allow to lock again the m_start_finished_mutex which is hold when the thread is killed. Closes: #6065
|
Looks good to me but I have no BSD setup to test on. |
|
It shouldn't require a BSD to test.
The testThreadKill tested that a Thread could be killed and restarted safely (something not used in minetest). But it was relying on undefined behaviour for that: the fact that a random thread could unlock a mutex hold by a cancelled thread (something forbidden on BSD, but allowed with glibc on Linux).
Now that the `kill()` method is merged in the destructor, it is explicit that a Thread is killed only in the destructor: the thread couldn't be reused.
|
nerzhul
requested changes
Sep 8, 2020
|
as you are in destructor, you can drop all asignment there because after destructor we won't read them :D
I was more concerned by concurrent access than about what can happens after destructor.
I don't know enough C++ to be sure that if the destructor is called it means really nothing has reference on it and could race with the code inside the destructor. So I was conservative.
|
sfan5
changed the title
pyrollo
reviewed
Sep 9, 2020
src/threading/thread.cpp
Outdated
Comment on lines
96
to
98
| m_retval = nullptr; | ||
| m_joinable = false; | ||
| m_request_stop = false; |
There was a problem hiding this comment.
Same as Nerzhul told above, these assignments are useless now.
|
Ok for me once superfluous assignments removed. |
|
I removed the assigments done after killing the thread, but keep the one before
(m_running = false), as in !_WIN32 case, the code will call `wait()` function,
and there is an assert on `m_running`.
|
|
Performing a test on (virtual) Windows before merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Thread kill() method isn't used outside testThreadKill unit test
and Thread destructor. It could be make private.
The testThreadKill test should be removed: it tests the kill() feature
in a way minetest doesn't use it (kill and restart a thread), specially
because some pthreads implementations on BSD will not allow to lock
again the m_start_finished_mutex which is hold when the thread is
killed.
Closes: #6065
This PR is Ready for Review.
Eventually, the
Thread::kill()method could be merged insideThread::~Thread()