Fix for issue #1167, added handling of errors which are returned by scaner #1253
Conversation
| if err := stdOutScanner.Err(); err != nil { | ||
| if err == bufio.ErrTooLong { | ||
| reader := bufio.NewReader(e.stdout) | ||
| log, _, _ := reader.ReadLine() |
There was a problem hiding this comment.
Why Readline vs ReadString('\n')?
There was a problem hiding this comment.
I don't see any special reasons to use Readline() instead of ReadString('\n'). I changed Readline()to ReadString('\n') to avoid parsing to string.
| @@ -115,6 +115,7 @@ func (e *ExecutablePlugin) Run(timeout time.Duration) (Response, error) { | |||
| e.cmd.Start() | |||
| e.captureStderr() | |||
| go func() { | |||
| OK: | |||
There was a problem hiding this comment.
I think this would be clearer as a loop vs a goto.
There was a problem hiding this comment.
OK, I changed it, now loops are in use.
| if c, ok := mts[i].Config().Table()["long_stderr_log"]; ok && c.(ctypes.ConfigValueBool).Value { | ||
| letterBytes := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" | ||
| longLine := []byte{} | ||
| for i := 0; i < bufio.MaxScanTokenSize; i++ { |
There was a problem hiding this comment.
Is the goal here to append to longline until it is larger than the MaxScanTokenSize?
There was a problem hiding this comment.
Here the goal is to have longline with length equal to MaxScanTokenSize. The condition in Scan() method has following form:
if len(s.buf) >= s.maxTokenSize || len(s.buf) > maxInt/2 {
s.setErr(ErrTooLong)
return false
}
katarzyna-z
force-pushed
the
fix-issue#1167
branch
4 times, most recently
from
1bc76c1 to
d30b49f
Compare
| WithField("scanner_err", stdOutScanner.Err()). | ||
| WithField("read_string_err", err). | ||
| Warn(log) | ||
| continue |
There was a problem hiding this comment.
I don't think this continue is needed.
Also there is still a case where we will want to break out of the loop right? Otherwise this goroutine will never exit?
There was a problem hiding this comment.
I made a small correction in code to finish the loop after scanning. Currently, I don't see solution without continue. Please take a look at this. If you have another idea, I'm open for your suggestions.
There was a problem hiding this comment.
In the iteration I was commenting on the continue served no purpose as there wasn't code executed after it. The update to add break below it changes that behavior.
katarzyna-z
force-pushed
the
fix-issue#1167
branch
from
d30b49f to
61f9c8d
Compare
|
|
||
| if err := stdErrScanner.Err(); err != nil { | ||
| reader := bufio.NewReader(e.stderr) | ||
| log, err := reader.ReadString('\n') |
There was a problem hiding this comment.
we need to handle the err from ReadString
log, err2 := reader.ReadString('\n')
if err2 == io.EOF {
break
}
Without the above block when the plugin stops the go routine never exits. You can see this by loading the modified mock2 plugin and starting the following task and then finally stop (ctrl-c) snapd.
---
version: 1
schedule:
type: "simple"
interval: "1s"
max-failures: 10
workflow:
collect:
metrics:
/intel/mock/foo: {}
/intel/mock/bar: {}
config:
/intel/mock:
name: "root"
password: "secret"
long_stderr_log: true
| WithField("plugin", path.Base(e.cmd.Path())). | ||
| WithField("io", "stderr"). | ||
| WithField("scanner_err", stdErrScanner.Err()). | ||
| WithField("read_string_err", err). |
There was a problem hiding this comment.
No need to called stdErrScanner.Err() twice (use err).
|
|
||
| if err := stdOutScanner.Err(); err != nil { | ||
| reader := bufio.NewReader(e.stdout) | ||
| log, err := reader.ReadString('\n') |
There was a problem hiding this comment.
Same comments made below on the stderr routine apply here (re: handle ReadString error).
katarzyna-z
force-pushed
the
fix-issue#1167
branch
2 times, most recently
from
373f43b to
2283c8d
Compare
…urned by scanner
|
Thanks for review. I made a correction. Could you check it? |
|
LGTM. @jcooklin? |
Fixes #1167
Hanging of plugins during capturing of stderr and stdout were observed when log line had a significant length. It was caused by errors occured during scanning of stderr or stdout. Errors returned by Scan() were not handled by snapd.
To fix the issue used the method which was used before adding of log capturing from plugin commit so now goto procedure is in use to deal with long line of logs.
Summary of changes:
Testing done:
@intelsdi-x/snap-maintainers