fix: handle LineReader buffer wrapparound in LineProtocolFilter (#502) #503
Conversation
lorinescu
force-pushed
the
fix-lineprotocolfilter-truncation
branch
from
32604a1
to
bd47464
Compare
lorinescu
force-pushed
the
fix-lineprotocolfilter-truncation
branch
from
bd47464
to
7452099
Compare
There was a problem hiding this comment.
Thank you for your PR to the influx-cli project! Sorry it has taken so long to review it.
I have a few suggestions for the code which will improve readability, maintainability, and possibly efficiency. If you would like to make the modifications, I will happily re-review. Thanks, again!
| continue | ||
| bytesRead, err := state.lineReader.Read(state.line) | ||
| if err != nil && bytesRead == 0 { | ||
| return 0, err |
There was a problem hiding this comment.
This conditional changes the behavior; in the case where bytes are read, but an error is returned (perhaps EOF from a final line without a newline), this will continue, whereas the existing code will return the bytesRead and the error.
There was a problem hiding this comment.
Is this a fix for a problem you found, or an inadvertent change in behavior?
| @@ -21,24 +25,35 @@ func LineProtocolFilter(reader io.Reader) *LineProtocolFilterReader { | |||
| lineReader.LineNumber = 1 // start counting from 1 | |||
| return &LineProtocolFilterReader{ | |||
| lineReader: lineReader, | |||
| line: make([]byte, 4096), | |||
There was a problem hiding this comment.
4096 should be the defaultBufSize constant, not a literal
There was a problem hiding this comment.
Note: I am suggesting elsewhere that this buffer is unnecessary.
| @@ -21,24 +25,35 @@ func LineProtocolFilter(reader io.Reader) *LineProtocolFilterReader { | |||
| lineReader.LineNumber = 1 // start counting from 1 | |||
| return &LineProtocolFilterReader{ | |||
| lineReader: lineReader, | |||
| line: make([]byte, 4096), | |||
| wrapBuf: make([]byte, 4096), | |||
There was a problem hiding this comment.
4096 should be the defaultBufSize constant, not a literal
There was a problem hiding this comment.
Note: I am suggesting elsewhere that this buffer is not necessary.
|
|
||
| //read again when we read a partial line at the end of the line reader buffer | ||
| if bytesRead > 0 && state.line[bytesRead-1] != '\n' { | ||
| wrapBytesRead, _ := state.lineReader.Read(state.wrapBuf[0:]) |
There was a problem hiding this comment.
This code ignores any errors from lineReader.Read. Errors need to be handled and returned up the stack.
| return 0, nil | ||
| } | ||
|
|
||
| copy(b, state.line[0:bytesRead]) |
There was a problem hiding this comment.
This code has an extra copy for every read, and sometimes two extra copies on a line wrap.
Are those copies necessary? Would it be possible to do all the reads into the passed in buffer, b, calling the second read (when there is no \n found) b[bytesRead:] or something like that?
Fix for #502