go: encoding/json: incorrect usage of sync.Pool
https://golang.org/cl/84897 introduces a denial-of-service attack on json.Marshal via a live-lock situation with sync.Pool.
Consider this snippet:
type CustomMarshaler int
func (c CustomMarshaler) MarshalJSON() ([]byte, error) {
time.Sleep(500 * time.Millisecond) // simulate processing time
b := make([]byte, int(c))
b[0] = '"'
for i := 1; i < len(b)-1; i++ {
b[i] = 'a'
}
b[len(b)-1] = '"'
return b, nil
}
func main() {
processRequest := func(size int) {
json.Marshal(CustomMarshaler(size))
time.Sleep(1 * time.Millisecond) // simulate idle time
}
// Simulate a steady stream of infrequent large requests.
go func() {
for {
processRequest(1 << 28) // 256MiB
}
}()
// Simulate a storm of small requests.
for i := 0; i < 1000; i++ {
go func() {
for {
processRequest(1 << 10) // 1KiB
}
}()
}
// Continually run a GC and track the allocated bytes.
var stats runtime.MemStats
for i := 0; ; i++ {
runtime.ReadMemStats(&stats)
fmt.Printf("Cycle %d: %dB\n", i, stats.Alloc)
time.Sleep(time.Second)
runtime.GC()
}
}
This is a variation of https://github.com/golang/go/issues/23199#issuecomment-353193866 of a situation suggested by @bcmills.
Essentially, we have a 1-to-1000 ratio of a routines that either use 1KiB or 256MiB, respectively. The occasional insertion of a 256MiB buffer into the sync.Pool gets continually held by the 1KiB routines. On my machine, after 300 GC cycles, the above program occupies 6GiB of my heap, when I expect it to be 256MiB in the worst-case.
\cc @jnjackins @bradfitz
About this issue
- Original URL
- State: open
- Created 6 years ago
- Comments: 39 (20 by maintainers)
Commits related to this issue
- encoding/json: use append for Compact and Indent This is part of the effort to reduce direct reliance on bytes.Buffer so that we can use a buffer with better pooling characteristics. Avoid direct us... — committed to golang/go by dsnet a year ago
- encoding/json: unify encodeState.string and encodeState.stringBytes This is part of the effort to reduce direct reliance on bytes.Buffer so that we can use a buffer with better pooling characteristic... — committed to golang/go by dsnet a year ago
- encoding/json: use append-like operations for encoding As part of the effort to rely less on bytes.Buffer, switch most operations to use more natural append-like operations. This makes it easier to s... — committed to golang/go by dsnet a year ago
I’ve been thinking of ways to guarantee bounds on the worst-case behavior, and this is a prototype idea:
In the worst-case scenario, we can ensure a bound on the minimal utilization based on the % threshold (e.g., 50% in the example above) and maximum number of sequential recycles below that threshold (e.g., 4x in the example above).
For the constants chosen above, the worst case sequence of utilization would be:
On average, that’s a worst case utilization of 10%, which is far better than the theoretical worst-case of 0% if the code naively recycles buffers without any limits.
Advantages of the algorithm:
Happy to hear suggestions for improvements.
Another approach would be to remove the package level
encodeStatePooland allow the caller to optionally supply one when creating an encoder. The reasoning here is that the caller has more knowledge about the distribution of payloads and the edge cases that a package-level statistical count would struggle with.I’m still a bit saddened by the amount of code being added. I sent out https://go.dev/cl/471200 as a simpler alternative implementation.
Here’s the performance comparison:
where:
We see that both v1 and v2 result in low allocation wastage compared to v0. This is the problem we’re trying to solve.
Comparing performance of v2 relative to v1, we get:
There’s generally a performance improvement over the more complicated https://go.dev/cl/455776 implementation. The apparent performance degradation of
EncoderEncodeseems to be largely in the noise.I feel bad sending out an alternative CL after you (@lavalamp) clearly spent a lot of time on your CL. I’m okay with you taking attribution credit for the CL if you want. After all, my CL would not have come about until you spent the grunt work to prove that segmented buffers were a viable solution.
(The benchmarks for v2 were not run with the optimization to
bytes.Joinin https://go.dev/cl/456336)It’s not dropping 64k buffers, but always choosing to recycle them in the pool. That specific code path isn’t strictly necessary, but for buffers below a certain size it probably doesn’t hurt to always recycle them regardless of the utilization.
Interesting idea. I think a useful statistic to obtain would perhaps be the maximum length encountered over the last N usages. If we evict a buffer due to low utilization, we can allocate a new one based on a recently seen maximum length.