FIX: Address matrix race conditions with sync.Once#75
Merged
makiuchi-d merged 4 commits intomakiuchi-d:masterfrom Jul 20, 2025
Merged
FIX: Address matrix race conditions with sync.Once#75makiuchi-d merged 4 commits intomakiuchi-d:masterfrom
makiuchi-d merged 4 commits intomakiuchi-d:masterfrom
Conversation
Owner
|
Thank you for your great contribution! |
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.
@makiuchi-d first of all - thank you for maintaining gozxing!
It's wonderful to see a pure Go implementation and with such a clean API design, it's great to use! In that spirit, I hope you don't mind my submission for consideration.
Problem
While using gozxing in a high-throughput QR scanning environment,
BinaryBitmap.GetBlackMatrix()andHybridBinarizer.GetBlackMatrix()have race conditions when accessed concurrently by multiple goroutines.This prevents safely caching and reusing
BinaryBitmapinstances across requests.Why This Matters
Creating a
BinaryBitmapfrom an image can be computationally expensive:For services processing thousands of QR codes per second, caching these preprocessed bitmaps can provider real-world benefit. My ad hoc benchmarking here suggests:
However, with the code today, it appears that developers must choose between:
Proposed Solution
I think we can get the benefits of your great library here but extend its performance use cases. The approach I've taken is to use
sync.Onceto ensure thread-safe lazy initialization of the black matrix. This is a minimal change that (at least in my testing) preserves all existing behavior while adding thread safety.I've applied the same pattern to the
HybridBinarizer, of course.Verification
go test -race ./...shows no issuesBinaryBitmapdoes not suffer with my changesDemonstration
A complete comparison showing the race conditions and the fix is available at:
https://github.com/bashhack/gozxing-thread-safe-compare
I show there how this change:
Breaking Changes
None that I can tell - the behavior for single-threaded access remains identical. Fully backward compatible.
Checklist
go test -raceI'm happy to make any adjustments to better align with your project's goals. Thank you for considering this contribution!