net/textproto: seemingly unnecssary buffer copy and reassignment in Reader.ReadLineBytes

[odeke-em]

I was just studying up and working on some small net/http performance changes and while examining its dependencies stumbled upon net/textproto (*Reader).ReadLineBytes()

go/src/net/textproto/reader.go

Lines 40 to 49 in bc4a10d

	// ReadLineBytes is like ReadLine but returns a []byte instead of a string.
	func (r *Reader) ReadLineBytes() ([]byte, error) {
	line, err := r.readLineSlice()
	if line != nil {
	buf := make([]byte, len(line))
	copy(buf, line)
	line = buf
	}
	return line, err
	}

The source code was last edited 7 years ago in 27a3dcd and am wondering whether the code in question is just a vestige of the old days or if perhaps there is a subtle reason behind that copy and reassignment

if line != nil {
    buf := make([]byte, len(line))
    copy(buf, line)
    line = buf
}

I believe that code should just replace that for (*Reader) readLineSlice
that is

diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go
index feb464b2f2..83ecae6fb4 100644
--- a/src/net/textproto/reader.go
+++ b/src/net/textproto/reader.go
@@ -33,22 +33,12 @@ func NewReader(r *bufio.Reader) *Reader {
 // ReadLine reads a single line from r,
 // eliding the final \n or \r\n from the returned string.
 func (r *Reader) ReadLine() (string, error) {
-       line, err := r.readLineSlice()
+       line, err := r.ReadLineBytes()
        return string(line), err
 }
 
 // ReadLineBytes is like ReadLine but returns a []byte instead of a string.
 func (r *Reader) ReadLineBytes() ([]byte, error) {
-       line, err := r.readLineSlice()
-       if line != nil {
-               buf := make([]byte, len(line))
-               copy(buf, line)
-               line = buf
-       }
-       return line, err
-}
-
-func (r *Reader) readLineSlice() ([]byte, error) {
        r.closeDot()
        var line []byte
        for {
@@ -120,7 +110,7 @@ func (r *Reader) ReadContinuedLineBytes() ([]byte, error) {
 
 func (r *Reader) readContinuedLineSlice() ([]byte, error) {
        // Read the first line.
-       line, err := r.readLineSlice()
+       line, err := r.ReadLineBytes()
        if err != nil {
                return nil, err
        }
@@ -145,7 +135,7 @@ func (r *Reader) readContinuedLineSlice() ([]byte, error) {
 
        // Read continuation lines.
        for r.skipSpace() > 0 {
-               line, err := r.readLineSlice()
+               line, err := r.ReadLineBytes()
                if err != nil {
                        break
                }
@@ -479,7 +469,7 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) {
 
        // The first line cannot start with a leading space.
        if buf, err := r.R.Peek(1); err == nil && (buf[0] == ' ' || buf[0] == '\t') {
-               line, err := r.readLineSlice()
+               line, err := r.ReadLineBytes()
                if err != nil {
                        return m, err
                }

If there is a reason behind it, let's please document it with a comment. In the standard library I can't find any usage of ReadLineBytes so perhaps that might be the reason why no one had noticed?

/cc @bradfitz @rsc please feel free to correct me if am mistaken.

[acln0]

As far as I can tell, the slice returned by readLineSlice may be invalidated on the next read, since it may come directly from https://golang.org/pkg/bufio/#Reader.ReadLine. Presumably, ReadLineBytes does not want to burden callers by propagating this behavior, so it makes a copy.