[5.0] refactor threading of snapshot_scheduler_test
#1821
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
linh2931
approved these changes
Oct 25, 2023
There was a problem hiding this comment.
Great fix!
The failed to read response from monitor process failure now appears in another test: https://github.com/AntelopeIO/leap/actions/runs/6634407607/job/18023991204. I added some some debug logging in compile_monitor.cpp in oc_compile_monitor_debug branch, run CICD manually hoping to see some logs, but the logs I added did not show up. I thought the monitor process might have already exited before get_connection_to_compile_monitor was called.
I could not reproduce it locally.
heifner
approved these changes
Oct 25, 2023
This was referenced Oct 25, 2023
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.
In the process of investigating #1794 I got my build in to a state (via various compile options & compilers I guess) where
snapshot_scheduler_testwould consistently fail viaUpon closer look, some aspects of threading look invalid and racy in this test. After this refactor my troublesome build always passes. I'd like to consider this resolving #1794 until we know otherwise (we should know shortly 🙂)
In particular,
There is a race between the
app->exec()and thechain_plug->chain().block_start.connect()-- blocks could be created before that connection is made and thus missed by the connection. I resolved this by moving the connection to occur beforeapp->exec(). I believe this is where my local failure stumbled.pp->schedule_snapshot()andpp->get_snapshot_requests()touch variables that are not guarded by any mutex. These should only be called by the "main thread" (in this case theapp_thread) so wrap them in a.post()to the app.I also replaced the effectively
sleep(10)for a wait on block 20 (however long it takes).