Use bufio.Reader instead of bufio.Scanner for logger.Copier #13564
Conversation
|
you are awesome :) |
| reader := bufio.NewReader(src) | ||
|
|
||
| for { | ||
| line, err := reader.ReadString('\n') |
There was a problem hiding this comment.
Oh yeah, good point, I forgot that one existed. Will update.
burke
force-pushed
the
fix-memory-leak
branch
3 times, most recently
from
1ace45e to
4c515fe
Compare
| return | ||
| } | ||
| logrus.Errorf("Error scanning log stream: %s", err) | ||
| return |
There was a problem hiding this comment.
I think you can remove the double return by doing something like:
if err != io.EOF {
logrus.Errorf("Error scanning log stream: %s", err)
}
returnThere was a problem hiding this comment.
I have this sense that comparing nil with io.EOF at runtime causes a panic but I can't remember why... I'll give it a shot.
There was a problem hiding this comment.
Oh, yeah, nevermind, I see what you're saying.
There was a problem hiding this comment.
but you compared that err != nil in the line before.
|
LGTM |
|
I think this can go into 1.7, but feel free to remove it from the milestone. |
calavera
added
the
kind/bug
|
ping @unclejack |
|
This closes #13333 |
| reader := bufio.NewReader(src) | ||
|
|
||
| for { | ||
| lineAndNewline, err := reader.ReadBytes('\n') |
There was a problem hiding this comment.
I kinda liked your lightweight solution with slicing. I think might be:
line, err := reader.ReadBytes('\n')
line = line[:len(line)-1]
There was a problem hiding this comment.
So the problem I had, and the reason I changed the commit, is that ReadBytes can return an error and a partial output. I haven't tested this, but I'm assuming that if it returns a partial input, it will end with a valuable byte and not a newline -- so it's probably prudent to only trim the trailing character if it's a newline.
There was a problem hiding this comment.
Okay, anyway then you should reuse one variable :)
There was a problem hiding this comment.
Works for me. Changed.
When using a scanner, log lines over 64K will crash the Copier with bufio.ErrTooLong. Subsequently, the ioutils.bufReader will grow without bound as the logs are no longer being flushed to disk. Signed-off-by: Burke Libbey <burke.libbey@shopify.com>
|
LGTM |
Use bufio.Reader instead of bufio.Scanner for logger.Copier
|
cherry-picked |
When using a scanner, log lines over 64K will crash the Copier with bufio.ErrTooLong. Subsequently, the ioutils.bufReader will grow without bound as the logs are no longer being flushed to disk.
For us, this manifested as some background workers occasionally stopping log writes, and slow memory growth between deploys.