Refactor Encoder.WriteEntry => Encoder.EncodeEntry (#245)

* Refactor Encoder.{Write => Encode}Entry

* Just use final in jsonEncoder.EncodeEntry

* Add a short write test

* Fix dat package

* Drop AddSync from TestWriterFacility*

logger_test.go

@@ -247,7 +247,7 @@ func TestLoggerNoOpsDisabledLevels(t *testing.T) {
 	})
 }
 
-func TestLoggerWriteEntryFailure(t *testing.T) {
+func TestLoggerWriteFailure(t *testing.T) {
 	errSink := &testutils.Buffer{}
 	logger := New(
 		zapcore.WriterFacility(

zapcore/encoder.go

@@ -20,8 +20,6 @@
 
 package zapcore
 
-import "io"
-
 // ObjectEncoder is a strongly-typed, encoding-agnostic interface for adding a
 // map- or struct-like object to the logging context. Like maps, ObjectEncoders
 // aren't safe for concurrent use (though typical use shouldn't require locks).
@@ -54,15 +52,16 @@ type ArrayEncoder interface {
 // lower-allocation.
 //
 // Implementations of the ObjectEncoder interface's methods can, of course,
-// freely modify the receiver. However, the Clone and WriteEntry methods will
+// freely modify the receiver. However, the Clone and EncodeEntry methods will
 // be called concurrently and shouldn't modify the receiver.
 type Encoder interface {
 	ObjectEncoder
 
 	// Clone copies the encoder, ensuring that adding fields to the copy doesn't
 	// affect the original.
 	Clone() Encoder
-	// Write a log entry and fields to the supplied writer, along with any
-	// accumulated context.
-	WriteEntry(io.Writer, Entry, []Field) error
+
+	// EncodeEntry encodes an entry and fields, along with any accumulated
+	// context, into a byte buffer and returns it.
+	EncodeEntry(Entry, []Field) ([]byte, error)
 }

zapcore/facility.go

@@ -21,9 +21,13 @@
 package zapcore
 
 import (
+	"fmt"
+	"io"
 	"sync"
 	"time"
 
+	"go.uber.org/zap/internal/buffers"
+
 	"go.uber.org/atomic"
 )
 
@@ -67,7 +71,13 @@ func (iof *ioFacility) Check(ent Entry, ce *CheckedEntry) *CheckedEntry {
 }
 
 func (iof *ioFacility) Write(ent Entry, fields []Field) error {
-	if err := iof.enc.WriteEntry(iof.out, ent, fields); err != nil {
+	buf, err := iof.enc.EncodeEntry(ent, fields)
+	if err != nil {
+		return err
+	}
+	err = checkPartialWrite(iof.out, buf)
+	buffers.Put(buf)
+	if err != nil {
 		return err
 	}
 	if ent.Level > ErrorLevel {
@@ -267,3 +277,16 @@ func (s *sampler) Check(ent Entry, ce *CheckedEntry) *CheckedEntry {
 	}
 	return s.Facility.Check(ent, ce)
 }
+
+// checkPartialWrite writes to an io.Writer, and upgrades partial writes to an
+// error if no other write error occured.
+func checkPartialWrite(w io.Writer, buf []byte) error {
+	n, err := w.Write(buf)
+	if err != nil {
+		return err
+	}
+	if n != len(buf) {
+		return fmt.Errorf("incomplete write: only wrote %v of %v bytes", n, len(buf))
+	}
+	return nil
+}

zapcore/facility_test.go

@@ -113,13 +113,23 @@ func TestWriterFacilitySyncsOutput(t *testing.T) {
 	}
 }
 
-func TestWriterFacilityWriteEntryFailure(t *testing.T) {
+func TestWriterFacilityWriteFailure(t *testing.T) {
 	fac := WriterFacility(
 		NewJSONEncoder(testJSONConfig()),
-		Lock(AddSync(&testutils.FailWriter{})),
+		Lock(&testutils.FailWriter{}),
 		DebugLevel,
 	)
+	err := fac.Write(Entry{}, nil)
+	// Should log the error.
+	assert.Error(t, err, "Expected writing Entry to fail.")
+}
 
+func TestWriterFacilityShortWrite(t *testing.T) {
+	fac := WriterFacility(
+		NewJSONEncoder(testJSONConfig()),
+		Lock(&testutils.ShortWriter{}),
+		DebugLevel,
+	)
 	err := fac.Write(Entry{}, nil)
 	// Should log the error.
 	assert.Error(t, err, "Expected writing Entry to fail.")

zapcore/json_encoder.go

@@ -22,14 +22,12 @@ package zapcore
 
 import (
 	"encoding/json"
-	"errors"
-	"fmt"
-	"io"
 	"math"
 	"strconv"
-	"sync"
 	"time"
 	"unicode/utf8"
+
+	"go.uber.org/zap/internal/buffers"
 )
 
 const (
@@ -39,19 +37,6 @@ const (
 	_initialBufSize = 1024
 )
 
-var (
-	// errNilSink signals that Encoder.WriteEntry was called with a nil WriteSyncer.
-	errNilSink = errors.New("can't write encoded message a nil WriteSyncer")
-
-	// TODO: use the new buffers package instead of pooling encoders.
-	jsonPool = sync.Pool{New: func() interface{} {
-		return &jsonEncoder{
-			// Pre-allocate a reasonably-sized buffer for each encoder.
-			bytes: make([]byte, 0, _initialBufSize),
-		}
-	}}
-)
-
 // A JSONConfig sets configuration options for a JSON encoder.
 type JSONConfig struct {
 	MessageFormatter func(string) Field
@@ -75,10 +60,10 @@ type jsonEncoder struct {
 // pair) when unmarshaling, but users should attempt to avoid adding duplicate
 // keys.
 func NewJSONEncoder(cfg JSONConfig) Encoder {
-	enc := jsonPool.Get().(*jsonEncoder)
-	enc.truncate()
-	enc.JSONConfig = &cfg
-	return enc
+	return &jsonEncoder{
+		JSONConfig: &cfg,
+		bytes:      buffers.Get(),
+	}
 }
 
 func (enc *jsonEncoder) AddString(key, val string) {
@@ -159,29 +144,26 @@ func (enc *jsonEncoder) AppendBool(val bool) {
 }
 
 func (enc *jsonEncoder) Clone() Encoder {
-	clone := jsonPool.Get().(*jsonEncoder)
-	clone.truncate()
-	clone.bytes = append(clone.bytes, enc.bytes...)
-	clone.JSONConfig = enc.JSONConfig
+	clone := &jsonEncoder{JSONConfig: enc.JSONConfig}
+	clone.bytes = append(buffers.Get(), enc.bytes...)
 	return clone
 }
 
-func (enc *jsonEncoder) WriteEntry(sink io.Writer, ent Entry, fields []Field) error {
-	if sink == nil {
-		return errNilSink
+func (enc *jsonEncoder) EncodeEntry(ent Entry, fields []Field) ([]byte, error) {
+	final := &jsonEncoder{
+		JSONConfig: enc.JSONConfig,
+		bytes:      buffers.Get(),
 	}
 
-	final := jsonPool.Get().(*jsonEncoder)
-	final.truncate()
 	final.bytes = append(final.bytes, '{')
-	enc.LevelFormatter(ent.Level).AddTo(final)
-	enc.TimeFormatter(ent.Time).AddTo(final)
+	final.LevelFormatter(ent.Level).AddTo(final)
+	final.TimeFormatter(ent.Time).AddTo(final)
 	if ent.Caller.Defined {
 		// NOTE: we add the field here for parity compromise with text
 		// prepending, while not actually mutating the message string.
 		final.AddString("caller", ent.Caller.String())
 	}
-	enc.MessageFormatter(ent.Message).AddTo(final)
+	final.MessageFormatter(ent.Message).AddTo(final)
 	if len(enc.bytes) > 0 {
 		if len(final.bytes) > 1 {
 			// All the formatters may have been no-ops.
@@ -194,27 +176,13 @@ func (enc *jsonEncoder) WriteEntry(sink io.Writer, ent Entry, fields []Field) er
 		final.AddString("stacktrace", ent.Stack)
 	}
 	final.bytes = append(final.bytes, '}', '\n')
-
-	expectedBytes := len(final.bytes)
-	n, err := sink.Write(final.bytes)
-	final.free()
-	if err != nil {
-		return err
-	}
-	if n != expectedBytes {
-		return fmt.Errorf("incomplete write: only wrote %v of %v bytes", n, expectedBytes)
-	}
-	return nil
+	return final.bytes, nil
 }
 
 func (enc *jsonEncoder) truncate() {
 	enc.bytes = enc.bytes[:0]
 }
 
-func (enc *jsonEncoder) free() {
-	jsonPool.Put(enc)
-}
-
 func (enc *jsonEncoder) addKey(key string) {
 	enc.separateElements()
 	enc.bytes = append(enc.bytes, '"')

zapcore/json_encoder_bench_test.go

@@ -22,9 +22,10 @@ package zapcore
 
 import (
 	"encoding/json"
-	"io/ioutil"
 	"testing"
 	"time"
+
+	"go.uber.org/zap/internal/buffers"
 )
 
 func testJSONConfig() JSONConfig {
@@ -53,7 +54,6 @@ func BenchmarkJSONLogMarshalerFunc(b *testing.B) {
 			enc.AddInt64("i", int64(i))
 			return nil
 		}))
-		enc.free()
 	}
 }
 
@@ -71,11 +71,11 @@ func BenchmarkZapJSON(b *testing.B) {
 			enc.AddString("string3", "🤔")
 			enc.AddString("string4", "🙊")
 			enc.AddBool("bool", true)
-			enc.WriteEntry(ioutil.Discard, Entry{
+			buf, _ := enc.EncodeEntry(Entry{
 				Message: "fake",
 				Level:   DebugLevel,
 			}, nil)
-			enc.free()
+			buffers.Put(buf)
 		}
 	})
 }