-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement LWG-4301: condition_variable{_any}::wait_{for, until} should take timeout by value #5885
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
base: main
Are you sure you want to change the base?
Implement LWG-4301: condition_variable{_any}::wait_{for, until} should take timeout by value #5885
Conversation
|
Notes and questions for review: The behavior of most overloads stays identical, as they are all implemented in terms of other functions from the I have kept all absolute/relative timeouts as const arguments. It appears that some parts of the STL adhere to the "const everything" guideline, so I hope it is valid. While preparing the tests, I have found a difference in the behavior of MSVC and clang/GCC. Details are in #5859 (comment) |
|
Moving to WIP as there are test failures. |
…G-4301 from yvals_core.h
|
I have incorporated @frederick-vs-ja 's suggestions. However, regarding the failing tests, I am baffled a little. I can't reliably reproduce the failure - I reproduced it twice using |
|
Are there timing assumptions in your tests? Whenever exercising timeout-related code, we need to be very careful to write tests such that they always succeed, regardless of how quickly or slowly operations complete, limited only by the Standard's guarantees. We've had to fix/skip a lot of tests containing assumptions like "surely this will complete in 100ms" or "surely this will never take 2 seconds". |
It is It doesn't seem to have any timing assumptions. |
I have sprinkled the test with some assertions and I have pinpointed the issue - exactly as you suggested. I assumed the child thread is fired in less than 100 ms, whereas in reality, I have observed some very high delays (e.g. 5 seconds). I will add some startup synchronization to ensure the child thread is properly started; it is not very elegant in C++11 (without |
| // If the main thread wait times out after short time, the modification did not influence the ongoing wait | ||
| template <typename CV> | ||
| void test_timeout_immutable(int test_number, int retries_remaining = 5) { | ||
| printf("\ntest %d\n", test_number); |
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.
Need to include <cstdio>. I suppose these printfs are an acceptable exception to our usual conventions.
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.
They were very valuable when collecting test results, but I don't have strong feeling for them. I meant to remove them before pushing, but apparently I did not. If you say so, I can remove them. Especially when you suggest that usage of printf in tests is rare.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Change pass-by-const-reference to pass-by-value.
Add tests.
Fixes #5859.