Fix sleep_for for max duration #2601
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
taminob
force-pushed
the
bugfix/2598/fix-max-duration-sleep-for-bug
branch
2 times, most recently
from
74b27fb
to
c5b1703
Compare
|
This PR fixes the issue by using sleep_until to emulate sleep_for since sleep_for is defined as identical to adding the current time and then using sleep_until by the standard.
That is not entirely true. `std::condition_variable::wait_for` at
least in GCC libstdc++ on GNU libc defaults to
`pthread_cond_clockwait` with monotonic clock
(`std::chrono::clock_steady`), protecting it from system clock
adjustments. The code here passes `std::chrono::system_clock` to
`wait_until` and thus the sleep duration will be affected by the
system clock changes.
|
Good point, thanks for the correction. Tomorrow, I'll also split this PR up into the testing/build system changes (since it's not really related to the change anyway) and the fix + re-applied PR. |
The standard library has the implicit requirement that for std::condition_variable::sleep_for() the duration must not cause an overflow if added to the current time. This commit will reduce the duration accordingly to fit into the duration type.
In the previous fix for a passed max duration, the assumption was made that at maximum one second will pass between the duration assignment and the std::condition_variable::sleep_for() call. This implementation makes the behavior more predictable by using sleep_until() instead to emulate the sleep_for() behavior.
This reverts commit 2d33c20 and reapplies various patches for memory leaks. The reason for the revert was a bug for a maximum duration interval which caused sleep_for() to cause unpredictable behavior.
Using pascal case for the class name keeps it more consistent with the majority of the other class names.
taminob
force-pushed
the
bugfix/2598/fix-max-duration-sleep-for-bug
branch
from
c5b1703
to
68dfd6a
Compare
|
Thanks :) |
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.
As mentioned in the issue, the bug is caused by the standard library expecting for
std::condition_variable::sleep_fora duration which does not cause an overflow when added to the current time.This PR fixes the issue by using
sleep_untilto emulatesleep_forsincesleep_foris defined as identical to adding the current time and then usingsleep_untilby the standard.Additionally, this PR re-applies #2586 which was reverted in #2599 because of this bug.
I also tried to write a test to catch this kind of error for
SleeperThread, but I wasn't able to check for this kind of failure.The standard does not specify the behavior if a too big duration is passed.
I ended up writing a few basic other tests for the class and restructured the build system to allow for easier addition to the tests by providing all sources of
waybaras a static library which is now simply linked againstmain.cppfor the executable.Resolves #2598