fix bufio.Scanner: token too long #99
Conversation
There was a problem hiding this comment.
Hi @everesio thank you for the patch. I noticed that you did not include a link to an issue. Can you expand your PR description with an example of the error being produced by a function? Ideally, with a link or zip of the function code so that we can manually verify this fix?
Finally, I took a look at the docs for Scanner and you could reuse the scanner implementation by setting the scanner.Buffer(buf, 64*1024) and I believe it would achieve the same effect. See https://golang.org/pkg/bufio/#Scanner.Buffer. Also, according to the documentation, scanner already uses a default buffer of 64*1024 see the docs for scanner.Buffer and here https://golang.org/pkg/bufio/#pkg-constants
A reproducible example would really help us merge this, thanks.
executor/logging.go
Outdated
| @@ -10,15 +10,21 @@ import ( | |||
| func bindLoggingPipe(name string, pipe io.Reader, output io.Writer) { | |||
| log.Printf("Started logging %s from function.", name) | |||
|
|
|||
| scanner := bufio.NewScanner(pipe) | |||
| reader := bufio.NewReaderSize(pipe, 64*1024) | |||
There was a problem hiding this comment.
can we add a comment explaining this number 64*1024 the next person to edit this might not know why it should be that value and not something longer or shorter
There was a problem hiding this comment.
I've created a pipeReaderBufSize constant with bufio.MaxScanTokenSize as value, as I wanted to keep the same max buffer size as Scanner. I hope this is less cryptic.
executor/logging_test.go
Outdated
| "time" | ||
| ) | ||
|
|
||
| func TestBindLoggingPipe(t *testing.T) { |
There was a problem hiding this comment.
This test doesn't really explain what it is testing, i think the name should be something like TestBindLoggingPipeHandlesLongLines this is much clearer about the intent and if anyone needs to edit it in the future, it will be very clear what the behavior should be. Right now the name just tells us what method is called, but not what behavior is being covered.
executor/logging_test.go
Outdated
| defer close(done) | ||
| max := 4 | ||
| for i := 0; i < max; i++ { | ||
| _, err := w.Write([]byte(fmt.Sprintf("%s\n", strings.Repeat("x", ssize)))) |
There was a problem hiding this comment.
To property test the behavior, shouldn't ssize be larger that the constant you set here https://github.com/openfaas-incubator/of-watchdog/pull/99/files#diff-42b34d1af16e4b2190c38ad0ec0e0f54R13
|
|
||
| func TestBindLoggingPipe(t *testing.T) { | ||
|
|
||
| r, w := io.Pipe() |
There was a problem hiding this comment.
both r and w are Closers. For completeness you should call r.Close() and w.Close() in this test
|
|
||
| select { | ||
| case <-done: | ||
| return |
There was a problem hiding this comment.
you never read the log content. when this is done you should not return but instead check that the content was written correctly
buf := new(bytes.Buffer)
buf.ReadFrom(r)
logLines := buf.String()
if logLines != expectedLogs {
t.Fatalf("unexpected logs lines, expected %s, got %s", expectedLogs, logLines)
}
executor/logging_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| time.Sleep(duration / time.Duration(int64(max))) |
|
/add label: design/review |
|
/add label: not approved |
|
My thoughts are the same as what Lucas said initially, we need to understand the use-case here. Thanks for raising an issue, but it's still lacking some details about the problem being solved. Alex |
everesio
force-pushed
the
token-too-long
branch
4 times, most recently
from
627bc3e
to
d9f6d7a
Compare
|
Please hold off on further work. This PR is not approved and we have an open discussion on the issue. |
'Error scanning stderr: bufio.Scanner: token too long' when a line is longer than 64kB. Subsequent writes to pipe hang. Signed-off-by: Michal Budzyn <michalbudzyn@gmail.com>
|
I opened the #100 and added the test handler. |
|
Hi @alexellis, I am a little confused. How do you want to proceed with the PR or solving the issue in other way ? Regards, |
|
I'm trying to understand why there is a log line of 64kb. I'll take a look at your issue to see if it's been updated yet. |
|
Description
Replace bufio.NewScanner by bufio.NewReaderSize as the scanner returns "token too long" error when the line is longer than 65536 bytes
Motivation and Context
When function receives and logs long input, the "bufio.Scanner: token too long" error is logged and the request processing is stopped.
How Has This Been Tested?
go test was added. function built with custom image everesio/of-watchdog:0.7.7-fix is not broken any more.
Types of changes
Checklist:
git commit -s