[Core][Tests] Cleanup HTTPServer start and stop #2867
Merged
Conversation
This file contains hidden or 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
panleone
reviewed
May 22, 2023
panleone
left a comment
panleone
left a comment
There was a problem hiding this comment.
Just one comment but overall I like the PR
Threads are still considered active, even if code is finished executing before being joined, and we do not want to try to join them if the code is not finished executing. Make sure queue empty before close Fix queue
Liquid369
force-pushed
the
2023_httpserver_stop
branch
from
88cefb5 to
9d42360
Compare
Liquid369
force-pushed
the
2023_httpserver_stop
branch
from
eeb56df to
9d42360
Compare
panleone
approved these changes
Jun 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Lately, we have been spending a lot of time messing with our test cases. We are hitting random errors at various points and it is causing us to waste time constantly fixing and re-running tests in order to be 'ready for review'. After discussion with the other developers, I went and looked into the code and found some tiny improvements that can assist us in not dealing with these cases anymore.
Threads are still considered active, even if the code is finished executing before being joined, and we do not want to try to join them if the code is not finished executing. So we are now checking if the threads can be closed and we are doing a little bit better cleanup of 0 > nullptr. Lastly, we want to make sure any queue is empty before closing.
To test:
The best route is to fork this branch and base it over your master or any branch you can run GitHub actions on, and run the tests several times, as well as running the test-runner.py on your local PC.
I have run over 50+ iterations of the tests over the weekend and have not encountered an issue in shutdown since.
We still have some issues in
tier_twotests that may appear, but evaluate the logs for whether or not you are incurring an issue from thestop_node()function.