appender: fix incorrect compare_exchange when rolling #1989
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
This reproduces the bug described in #1987 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch fixes a bug in `RollingFileAppender` where `compare_exchange` is called with the _current timestamp_ as the "current value" of the next rollover timestamp, rather than the actual current value. This means that if the current time is *greater* than the rollover time, the CAS will fail and the appender will never roll over --- currently, rolling only functions correctly if we try to write to the file at *precisely* the rollover time. This means that, in practice, the appender almost never rolls over. I've fixed this by ensuring that the compare-and-swap is always performed with the current value of the atomic, rather than the current timestamp. `should_rollover` now returns an `Option` with the current value in it to indicate it's time to roll over, so that we perform the CAS with the value loaded in `should_rollover`. Fixes #1987
davidbarsky
approved these changes
Mar 12, 2022
davidbarsky
pushed a commit
that referenced
this pull request
Mar 17, 2022
This branch fixes a bug in `RollingFileAppender` where `compare_exchange` is called with the _current timestamp_ as the "current value" of the next rollover timestamp, rather than the actual current value. This means that if the current time is *greater* than the rollover time, the CAS will fail and the appender will never roll over --- currently, rolling only functions correctly if we try to write to the file at *precisely* the rollover time. This means that, in practice, the appender almost never rolls over. I've fixed this by ensuring that the compare-and-swap is always performed with the current value of the atomic, rather than the current timestamp. `should_rollover` now returns an `Option` with the current value in it to indicate it's time to roll over, so that we perform the CAS with the value loaded in `should_rollover`. I've also added a test that exercises a file rollover using a mock time. This would have caught the bug described in #1987. Fixes #1987 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Merged
hawkw
added a commit
that referenced
this pull request
Mar 17, 2022
This branch fixes a bug in `RollingFileAppender` where `compare_exchange` is called with the _current timestamp_ as the "current value" of the next rollover timestamp, rather than the actual current value. This means that if the current time is *greater* than the rollover time, the CAS will fail and the appender will never roll over --- currently, rolling only functions correctly if we try to write to the file at *precisely* the rollover time. This means that, in practice, the appender almost never rolls over. I've fixed this by ensuring that the compare-and-swap is always performed with the current value of the atomic, rather than the current timestamp. `should_rollover` now returns an `Option` with the current value in it to indicate it's time to roll over, so that we perform the CAS with the value loaded in `should_rollover`. I've also added a test that exercises a file rollover using a mock time. This would have caught the bug described in #1987. Fixes #1987 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
pushed a commit
that referenced
this pull request
Mar 17, 2022
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.
This branch fixes a bug in
RollingFileAppenderwherecompare_exchangeis called with the current timestamp as the"current value" of the next rollover timestamp, rather than the actual
current value. This means that if the current time is greater than the
rollover time, the CAS will fail and the appender will never roll over
--- currently, rolling only functions correctly if we try to write to
the file at precisely the rollover time. This means that, in practice,
the appender almost never rolls over.
I've fixed this by ensuring that the compare-and-swap is always
performed with the current value of the atomic, rather than the current
timestamp.
should_rollovernow returns anOptionwith the currentvalue in it to indicate it's time to roll over, so that we perform the
CAS with the value loaded in
should_rollover.I've also added a test that exercises a file rollover using a mock
time. This would have caught the bug.
Fixes #1987