Fix a race condition in vmaf_thread_pool_wait. #894
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
There may still be work in the queue but n_working=0 before the vmaf_thread_pool_runner loop fetches another job in the queue. When calling vmaf_thread_pool_wait (and not stopping), we will still want to drain the queue. So only exit the wait loop when n_working = 0 && head == nullptr
|
Thank you, I will review this afternoon. Did you happen to hit this bug? |
|
No rush. It is possible to avoid the issue while using the non-threaded
code path (using threads=0).
And yes, I did encounter this issue. I noticed some high test flakiness
that hadn't been there in 1.x versions. In one case it was close to 40%
test failure, depending on compilation options, sanitizer options, etc (in
general, rate went down in debug and other sanitizer modes). I'm not
entirely sure why the test environment failures were so much more common
than the shell commands, however. I tested over 20K runs with the patch in
our test environment, and they all succeed.
Has any one else encountered / reported any similar flakiness?
…On Wed, Aug 11, 2021, 12:35 PM Kyle Swanson ***@***.***> wrote:
Thank you, I will review this afternoon. Did you happen to hit this bug?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#894 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPQAEKZMCAV3BLZFZNTXLDT4LGJJANCNFSM5B6XVSPA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
nbirkbeck
commented
Aug 11, 2021
| @@ -156,7 +156,8 @@ int vmaf_thread_pool_wait(VmafThreadPool *pool) | |||
| if (!pool) return -EINVAL; | |||
|
|
|||
| pthread_mutex_lock(&(pool->queue.lock)); | |||
| while((!pool->stop && pool->n_working) || (pool->stop && pool->n_threads)) | |||
| while((!pool->stop && (pool->n_working || pool->queue.head)) || | |||
There was a problem hiding this comment.
Just noting that it is probably logically sufficient to just check pool->queue.head, although I'll wait for someone closer to the code to comment.
(If there are other potential fixes, I can test in an environment where failures are likley)
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.
Fixes #893.
There may still be work in the queue but n_working=0 before the vmaf_thread_pool_runner loop fetches another job in the queue. When calling vmaf_thread_pool_wait (and not stopping), we will still want to drain the queue. So only exit the wait loop when n_working = 0 && head == nullptr