Fix usage of sync.Pool

The current usage of sync.Pool is leaky because it stores an arbitrary sized buffer into the pool. However, sync.Pool assumes that all items in the pool are interchangeable from a memory cost perspective. Due to the unbounded size of a buffer that may be added, it is possible for the pool to eventually pin arbitrarily large amounts of memory in a live-lock situation. As a simple fix, we just set a maximum size that we permit back into the pool.
markdessain · Sep 19, 2018 · e0f8de6 · e0f8de6
1 parent 972f271
commit e0f8de6
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 12 deletions.
diff --git a/array.go b/array.go
@@ -20,6 +20,20 @@ type Array struct {
 	buf []byte
 }
 
+func putArray(a *Array) {
+	// Proper usage of a sync.Pool requires each entry to have approximately
+	// the same memory cost. To obtain this property when the stored type
+	// contains a variably-sized buffer, we add a hard limit on the maximum buffer
+	// to place back in the pool.
+	//
+	// See https://golang.org/issue/23199
+	const maxSize = 1 << 16 // 64KiB
+	if cap(a.buf) > maxSize {
+		return
+	}
+	arrayPool.Put(a)
+}
+
 // Arr creates an array to be added to an Event or Context.
 func Arr() *Array {
 	a := arrayPool.Get().(*Array)
@@ -38,7 +52,7 @@ func (a *Array) write(dst []byte) []byte {
 		dst = append(append(dst, a.buf...))
 	}
 	dst = enc.AppendArrayEnd(dst)
-	arrayPool.Put(a)
+	putArray(a)
 	return dst
 }
 
@@ -49,7 +63,7 @@ func (a *Array) Object(obj LogObjectMarshaler) *Array {
 	obj.MarshalZerologObject(e)
 	e.buf = enc.AppendEndMarker(e.buf)
 	a.buf = append(enc.AppendArrayDelim(a.buf), e.buf...)
-	eventPool.Put(e)
+	putEvent(e)
 	return a
 }
 
@@ -80,7 +94,7 @@ func (a *Array) Err(err error) *Array {
 		e.buf = e.buf[:0]
 		e.appendObject(m)
 		a.buf = append(enc.AppendArrayDelim(a.buf), e.buf...)
-		eventPool.Put(e)
+		putEvent(e)
 	case error:
 		a.buf = enc.AppendString(enc.AppendArrayDelim(a.buf), m.Error())
 	case string:

diff --git a/context.go b/context.go
@@ -26,7 +26,7 @@ func (c Context) Fields(fields map[string]interface{}) Context {
 func (c Context) Dict(key string, dict *Event) Context {
 	dict.buf = enc.AppendEndMarker(dict.buf)
 	c.l.context = append(enc.AppendKey(c.l.context, key), dict.buf...)
-	eventPool.Put(dict)
+	putEvent(dict)
 	return c
 }
 
@@ -55,7 +55,7 @@ func (c Context) Object(key string, obj LogObjectMarshaler) Context {
 	e := newEvent(levelWriterAdapter{ioutil.Discard}, 0)
 	e.Object(key, obj)
 	c.l.context = enc.AppendObjectData(c.l.context, e.buf)
-	eventPool.Put(e)
+	putEvent(e)
 	return c
 }
 
@@ -64,7 +64,7 @@ func (c Context) EmbedObject(obj LogObjectMarshaler) Context {
 	e := newEvent(levelWriterAdapter{ioutil.Discard}, 0)
 	e.EmbedObject(obj)
 	c.l.context = enc.AppendObjectData(c.l.context, e.buf)
-	eventPool.Put(e)
+	putEvent(e)
 	return c
 }
 

diff --git a/diode/diode.go b/diode/diode.go
@@ -86,6 +86,16 @@ func (dw Writer) poll() {
 		}
 		p := *(*[]byte)(d)
 		dw.w.Write(p)
-		bufPool.Put(p[:0])
+
+		// Proper usage of a sync.Pool requires each entry to have approximately
+		// the same memory cost. To obtain this property when the stored type
+		// contains a variably-sized buffer, we add a hard limit on the maximum buffer
+		// to place back in the pool.
+		//
+		// See https://golang.org/issue/23199
+		const maxSize = 1 << 16 // 64KiB
+		if cap(p) <= maxSize {
+			bufPool.Put(p[:0])
+		}
 	}
 }
diff --git a/event.go b/event.go
@@ -33,6 +33,20 @@ type Event struct {
 	ch    []Hook // hooks from context
 }
 
+func putEvent(e *Event) {
+	// Proper usage of a sync.Pool requires each entry to have approximately
+	// the same memory cost. To obtain this property when the stored type
+	// contains a variably-sized buffer, we add a hard limit on the maximum buffer
+	// to place back in the pool.
+	//
+	// See https://golang.org/issue/23199
+	const maxSize = 1 << 16 // 64KiB
+	if cap(e.buf) > maxSize {
+		return
+	}
+	eventPool.Put(e)
+}
+
 // LogObjectMarshaler provides a strongly-typed and encoding-agnostic interface
 // to be implemented by types used with Event/Context's Object methods.
 type LogObjectMarshaler interface {
@@ -66,7 +80,7 @@ func (e *Event) write() (err error) {
 			_, err = e.w.WriteLevel(e.level, e.buf)
 		}
 	}
-	eventPool.Put(e)
+	putEvent(e)
 	return
 }
 
@@ -141,7 +155,7 @@ func (e *Event) Dict(key string, dict *Event) *Event {
 	}
 	dict.buf = enc.AppendEndMarker(dict.buf)
 	e.buf = append(enc.AppendKey(e.buf, key), dict.buf...)
-	eventPool.Put(dict)
+	putEvent(e)
 	return e
 }
 

diff --git a/fields.go b/fields.go
@@ -20,7 +20,7 @@ func appendFields(dst []byte, fields map[string]interface{}) []byte {
 			e.buf = e.buf[:0]
 			e.appendObject(val)
 			dst = append(dst, e.buf...)
-			eventPool.Put(e)
+			putEvent(e)
 			continue
 		}
 		switch val := val.(type) {
@@ -36,7 +36,7 @@ func appendFields(dst []byte, fields map[string]interface{}) []byte {
 				e.buf = e.buf[:0]
 				e.appendObject(m)
 				dst = append(dst, e.buf...)
-				eventPool.Put(e)
+				putEvent(e)
 			case error:
 				dst = enc.AppendString(dst, m.Error())
 			case string:
@@ -54,7 +54,7 @@ func appendFields(dst []byte, fields map[string]interface{}) []byte {
 					e.buf = e.buf[:0]
 					e.appendObject(m)
 					dst = append(dst, e.buf...)
-					eventPool.Put(e)
+					putEvent(e)
 				case error:
 					dst = enc.AppendString(dst, m.Error())
 				case string: