Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit 0e24a83

Browse files
Random-LiuRandom-Liu
committed
Fix the log ending newline handling.
Signed-off-by: Lantao Liu <[email protected]>
1 parent 562eefa commit 0e24a83

2 files changed

Lines changed: 83 additions & 10 deletions

File tree

‎pkg/server/io/logger.go‎

Lines changed: 69 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package io
1919
import (
2020
"bufio"
2121
"bytes"
22+
"fmt"
2223
"io"
2324
"io/ioutil"
2425
"time"
@@ -61,6 +62,56 @@ func NewCRILogger(path string, w io.Writer, stream StreamType, maxLen int) (io.W
6162
return pwc, stop
6263
}
6364

65+
// bufio.ReadLine in golang eats both read errors and tailing newlines
66+
// (See https://golang.org/pkg/bufio/#Reader.ReadLine). When reading
67+
// to io.EOF, it is impossible for the caller to figure out whether
68+
// there is a newline at the end, for example:
69+
// 1) When reading "CONTENT\n", it returns "CONTENT" without error;
70+
// 2) When reading "CONTENT", it also returns "CONTENT" without error.
71+
//
72+
// To differentiate these 2 cases, we need to write a readLine function
73+
// ourselves to not ignore the error.
74+
//
75+
// The code is similar with https://golang.org/src/bufio/bufio.go?s=9537:9604#L359.
76+
// The only difference is that it returns all errors from `ReadSlice`.
77+
//
78+
// readLine returns err != nil if and only if line does not end with a new line.
79+
func readLine(b *bufio.Reader) (line []byte, isPrefix bool, err error) {
80+
line, err = b.ReadSlice('\n')
81+
if err == bufio.ErrBufferFull {
82+
// Handle the case where "\r\n" straddles the buffer.
83+
if len(line) > 0 && line[len(line)-1] == '\r' {
84+
// Unread the last '\r'
85+
if err := b.UnreadByte(); err != nil {
86+
panic(fmt.Sprintf("invalid unread %v", err))
87+
}
88+
line = line[:len(line)-1]
89+
}
90+
return line, true, nil
91+
}
92+
93+
if len(line) == 0 {
94+
if err != nil {
95+
line = nil
96+
}
97+
return
98+
}
99+
100+
if line[len(line)-1] == '\n' {
101+
// "ReadSlice returns err != nil if and only if line does not end in delim"
102+
// (See https://golang.org/pkg/bufio/#Reader.ReadSlice).
103+
if err != nil {
104+
panic(fmt.Sprintf("full read with unexpected error %v", err))
105+
}
106+
drop := 1
107+
if len(line) > 1 && line[len(line)-2] == '\r' {
108+
drop = 2
109+
}
110+
line = line[:len(line)-drop]
111+
}
112+
return
113+
}
114+
64115
func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxLen int) {
65116
defer rc.Close()
66117
var (
@@ -88,7 +139,16 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
88139
}
89140
for {
90141
var stop bool
91-
newLine, isPrefix, err := r.ReadLine()
142+
newLine, isPrefix, err := readLine(r)
143+
// NOTE(random-liu): readLine can return actual content even if there is an error.
144+
if len(newLine) > 0 {
145+
// Buffer returned by ReadLine will change after
146+
// next read, copy it.
147+
l := make([]byte, len(newLine))
148+
copy(l, newLine)
149+
buf = append(buf, l)
150+
length += len(l)
151+
}
92152
if err != nil {
93153
if err == io.EOF {
94154
logrus.Debugf("Getting EOF from stream %q while redirecting to log file %q", s, path)
@@ -101,13 +161,6 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
101161
}
102162
// Stop after writing the content left in buffer.
103163
stop = true
104-
} else {
105-
// Buffer returned by ReadLine will change after
106-
// next read, copy it.
107-
l := make([]byte, len(newLine))
108-
copy(l, newLine)
109-
buf = append(buf, l)
110-
length += len(l)
111164
}
112165
if maxLen > 0 && length > maxLen {
113166
exceedLen := length - maxLen
@@ -125,7 +178,14 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
125178
if isPrefix {
126179
continue
127180
}
128-
writeLine(full, bytes.Join(buf, nil))
181+
if stop {
182+
// readLine only returns error when the message doesn't
183+
// end with a newline, in that case it should be treated
184+
// as a partial line.
185+
writeLine(partial, bytes.Join(buf, nil))
186+
} else {
187+
writeLine(full, bytes.Join(buf, nil))
188+
}
129189
buf = nil
130190
length = 0
131191
if stop {

‎pkg/server/io/logger_test.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestRedirectLogs(t *testing.T) {
7272
maxLen: maxLen,
7373
tag: []runtime.LogTag{
7474
runtime.LogTagFull,
75-
runtime.LogTagFull,
75+
runtime.LogTagPartial,
7676
},
7777
content: []string{
7878
"test stderr log 1",
@@ -222,6 +222,19 @@ func TestRedirectLogs(t *testing.T) {
222222
strings.Repeat("a", defaultBufSize*10+20),
223223
},
224224
},
225+
"log length longer than buffer size with tailing \\r\\n": {
226+
input: strings.Repeat("a", defaultBufSize-1) + "\r\n" + strings.Repeat("a", defaultBufSize-1) + "\r\n",
227+
stream: Stdout,
228+
maxLen: -1,
229+
tag: []runtime.LogTag{
230+
runtime.LogTagFull,
231+
runtime.LogTagFull,
232+
},
233+
content: []string{
234+
strings.Repeat("a", defaultBufSize-1),
235+
strings.Repeat("a", defaultBufSize-1),
236+
},
237+
},
225238
} {
226239
t.Logf("TestCase %q", desc)
227240
rc := ioutil.NopCloser(strings.NewReader(test.input))

0 commit comments

Comments
 (0)