fix: fixed race condition in clone - #11
Merged
Merged
Conversation
maoueh
reviewed
Aug 3, 2026
| }} | ||
|
|
||
| func getJSONEncoder() *jsonEncoder { | ||
| return _jsonPool.Get().(*jsonEncoder) |
Contributor
There was a problem hiding this comment.
Does this means then that the whole pool is now useless?
Ill need to verify if still used, otherwise we maybe better removing it entierly.
The `zap.Any`/`zap.Reflect` reflection buffer was never returned to the buffer pool. Its only release point, `putJSONEncoder`, is unreachable: `Encoder.EncodeEntry` shadows the embedded `jsonEncoder.EncodeEntry`, so that code never runs. Every log line carrying a reflected field therefore allocated a fresh buffer (1451 B/op, 13 allocs/op down to 392 B/op, 11 allocs/op, ~17% faster). For the same reason nothing ever fed `_jsonPool`, so `Get` always fell through to `New` and the pool could not have produced the clone race the previous commit targeted. Allocating explicitly is also measurably cheaper on the `Clone` path, which never hands its encoder back. Remove `_jsonPool`, `_loggerPool`, `putJSONEncoder` and the unused `truncate` / `ptrIntToString` helpers, and release the reflection buffer through a small `free` method instead. The concurrency test now asserts that every entry reaches the syncer and no longer claims to reproduce a race that it does not reproduce.
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.
Fix data race in
jsonEncoder.clone()due to pooled struct reuseProblem
go test -racereports a data race inside the encoder under concurrentzapcore.Encoder.Clone()/EncodeEntrycalls, e.g. two goroutines callingLogger.With(...)on the same base logger while log lines are emitted:Root cause
jsonEncoder.clone()fetches its receiver from async.Pool(_jsonPool)and then mutates it in place:
zapcore.Encoder.Clone()must return a deep, independent copy — zap may handthe same encoder to several
Clone()callers concurrently, and the wholepoint of
Cloneis that the returned encoder is not shared with the source.Reusing pooled structs breaks that contract: a struct that is still in use
as a live encoder can be handed out again by the pool and mutated, racing
the reads performed by the caller that still holds it.
The upstream zap implementation pools the same struct type, but its
EncodeEntryreturns every clone to the pool (putJSONEncoder(final)) inthe same goroutine that created it, so the reuse is tightly paired. Here the
outer
Encoder.Clone()allocates a fresh*Encoderanyway, so the innerpool provides no meaningful allocation win while introducing a shared
mutable state hazard.
Fix
Allocate a fresh
*jsonEncoderinclone()and drop the now-unusedgetJSONEncoder():The buffer pool (
bufferpool) is untouched: each clone still gets its ownbuffer via
bufferpool.Get(), which is safe.Verification
go test -race ./...passes (addedTestEncoderConcurrentWithAndLog,which hammers
Logger.With+ log emission from 8 goroutines on a sharedbase logger).
-raceflags the_jsonPool.New/clone()race under that pattern (and in a downstream consumer doing concurrent
.With()+ log emission).