fix(transformers): serialize UTCTimestampTransformer to avoid rng race#802
Open
SAY-5 wants to merge 3 commits into
Open
fix(transformers): serialize UTCTimestampTransformer to avoid rng race#802SAY-5 wants to merge 3 commits into
SAY-5 wants to merge 3 commits into
Conversation
greenmask's Timestamp holds a *rand.Rand that is not concurrency-safe, but pgstream shares one transformer across snapshot worker goroutines. Concurrent calls corrupted the shared rng and panicked deep in math/rand.(*rngSource).Uint64. Guard the Transform call with a per-instance mutex. Signed-off-by: SAY-5 <[email protected]>
Collaborator
|
Thank you for the contribution. We try to fix these issues in our fork or |
Author
|
Thanks for the pointer. The transformer surface in greenmask looks different from pgstream's, so the fix doesn't port 1:1, would you prefer I close this PR and let you sync the equivalent change into greenmask on the maintainers' next pass, or leave this open as a reference? |
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
2 participants
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.
Closes #789.
greenmask's
Timestamptransformer holds a*rand.Randinstance that's not safe for concurrent use, but pgstream shares oneUTCTimestampTransformeracross all snapshot worker goroutines. Concurrent calls corrupt the shared rng and panic deep insidemath/rand.(*rngSource).Uint64withindex out of range [-1].Guard the
Transformcall with a per-instance mutex. The same pattern likely applies to the other greenmask transformer wrappers in this package, happy to extend in a follow-up if you'd like, but kept this PR narrow to the surface in the bug report.