Fix: bitmatrix clone for concurrent decode#81
Open
bashhack wants to merge 1 commit intomakiuchi-d:masterfrom
Open
Fix: bitmatrix clone for concurrent decode#81bashhack wants to merge 1 commit intomakiuchi-d:masterfrom
bashhack wants to merge 1 commit intomakiuchi-d:masterfrom
Conversation
7cb0f3e to
e2a5d3b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #81 +/- ##
=======================================
Coverage 97.93% 97.93%
=======================================
Files 122 122
Lines 12951 12955 +4
=======================================
+ Hits 12684 12688 +4
Misses 191 191
Partials 76 76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 task
This file contains hidden or 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
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.
Hey @makiuchi-d - hope you're doing okay, apologies for the long gap between contributions - but I have one more to propose.
Summary
BitMatrixParsermutates its inputBitMatrixduring decoding (UnmaskBitMatrix,Remask,Mirror), but since #75 introducedsync.Oncecaching inBinaryBitmap.GetBlackMatrix(), all concurrent decoders share the same cached matrix instance. When multiple goroutines decode from the sameBinaryBitmap, one goroutine'sFlip()calls corrupt the matrix while another is reading, causing occasionalNotFoundExceptionwith invalid coordinates (e.g.(53, -2)).This then is a follow-up to my previous contribution #75, which fixed the
GetBlackMatrix()race but exposed this deeper mutation-sharing issue.Changes
BitMatrix.Clone(): Implements the deep copy method (stub comment already existed:// public BitMatrix clone()). Returns a newBitMatrixwith an independent copy of the backing[]uint32slice.NewBitMatrixParser: Clones the input matrix so that decode mutations (UnmaskBitMatrix,Remask,Mirror) operate on a private copy rather than the shared cached original.Why clone in the parser, not elsewhere?
sync.Oncecaching fromGetBlackMatrix()would work but forces expensive re-binarization on every call. Cloning aBitMatrixis a singlecopy()of the backing slice.DataMatrix'sBitMatrixParseris unaffected: it extracts a data region into a new matrix and never mutates the input.Behavioral change
Previously,
Decoder.Decode(bits, hints)mutated bits as a side effect (left it in an unmasked/mirrored state). Now bits is untouched after decode. This is arguably more correct, I think, as the mutations are implementation details of the decode retry logic, not a documented contract. No code in the repository reads theBitMatrixafter decoding.Testing
Updates and added tests, of course - but while I was here, I also tackled that
TestDefaultGridSampler_SampleGridfailure.