Fix panic introduced by OStream optimization #1437
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
richardartoul
force-pushed
the
ra/fix-panic
branch
2 times, most recently
from
85f11c9 to
794654c
Compare
richardartoul
force-pushed
the
ra/fix-panic
branch
from
1c73278 to
f2eb6d1
Compare
Codecov Report
@@ Coverage Diff @@
## master #1437 +/- ##
========================================
+ Coverage 70.9% 70.9% +<.1%
========================================
Files 836 836
Lines 71507 71507
========================================
+ Hits 50706 50714 +8
+ Misses 17507 17497 -10
- Partials 3294 3296 +2
Continue to review full report at Codecov.
|
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.
A panic was introduced by #1399 that can occur when many reads are happening in parallel for the same series. The panic occurs because the previous P.R introduced a called to
Reset()on achecked.Byteson the read path.The read path is only protected by an RLock so this would be interpreted by the checked.Bytes as a double write and cause a panic. Technically no double write was taking place because the second called to
Resetwould be a no-op, but the checked.Bytes has no way to distinguish that and we shouldn't be doing any mutation on the read path anyways.This P.R updates the code path so that the call to
RawBytesactually just returns[]byteinstead ofchecked.Bytes. This avoids the issue of performing a mutation on the read path entirely.I've also included a regression test which would have caught the issue.