Fix hangs when the reading from the native driver raised an error before all input was consumed #135
Conversation
|
Fixes bblfsh/bblfshd/issues/36. |
|
PS: we should really add a lot more verbose debug logging now that it's selectable. Maybe also set the default logging level to "info". It would make a lot easier to debug problems since when testing with the server and drivers in containers and gRPC calls you can't just use gdb. Should I open an issue? |
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
=========================================
+ Coverage 40.18% 40.5% +0.31%
=========================================
Files 21 21
Lines 2160 2185 +25
=========================================
+ Hits 868 885 +17
- Misses 1204 1212 +8
Partials 88 88
Continue to review full report at Codecov.
|
protocol/jsonlines/decoder.go
Outdated
| @@ -15,6 +14,7 @@ const ( | |||
|
|
|||
| type lineReader interface { | |||
| ReadLine() ([]byte, bool, error) | |||
| ReadSlice(delim byte) (line[] byte, err error) | |||
There was a problem hiding this comment.
Is it possible to replace ReadLine with ReadSlice instead of having both?
There was a problem hiding this comment.
Mmm, I think the code is simpler this way; ReadSlice returns (via parameter) a pointer (or pointers since it's a slice) to the original slice and don't block so it's perfect for the read-discard done on discardPending (no copying and no blocking). ReadLine on the other hand does a copy but also returns a specific code if the buffer is full so we can continue iterating by chunks, which it does on subsecuent calls.
Both could be implemented using the other but I think it's simpler that way in the places they're used here.
There was a problem hiding this comment.
I see, thanks for the explanation.
protocol/jsonlines/decoder.go
Outdated
| @@ -40,17 +40,41 @@ func NewDecoder(r io.Reader) Decoder { | |||
| return &decoder{r: lr} | |||
| } | |||
|
|
|||
| func (d *decoder) discardPending() error { | |||
| // Read and discard everytihng until the next newline | |||
There was a problem hiding this comment.
This line may be better above the function declaration, to document the function
protocol/jsonlines/decoder.go
Outdated
| // Read and discard everytihng until the next newline | ||
| for { | ||
| _, err := d.r.ReadSlice('\n') | ||
| if err != nil { |
There was a problem hiding this comment.
this nested if and if-else may get simpler with a switch
There was a problem hiding this comment.
Definitely! (too many years doing too much Python can do this to a programmer mind).
protocol/jsonlines/decoder.go
Outdated
| chunk, isPrefix, err := d.r.ReadLine() | ||
| if err != nil { | ||
| if !isPrefix { | ||
| d.discardPending() |
There was a problem hiding this comment.
return value of this call should be checked, shouldn't it?
There was a problem hiding this comment.
Mmm, it's already inside an error handler so my logic was to return the original error, but on a second tough I'll also check that error and produce a new error with a combined message since any error to discard the input buffer could bring more misteriously hanged containers.
There was a problem hiding this comment.
PS: is probably undefined behaviour to do this with the bufio object once it has already failed, but I checked causing the failure and without the discard the next call would hang so it's probably the lesser evil.
protocol/jsonlines/decoder.go
Outdated
| "io" | ||
| "fmt" |
protocol/jsonlines/decoder.go
Outdated
| var line []byte | ||
|
|
||
| for { | ||
| chunk, isPrefix, err := d.r.ReadLine() |
There was a problem hiding this comment.
Maybe move this code block to a smaller function (e.g. readLine(r lineReader) error) to avoid too much nesting and improve readability.
There was a problem hiding this comment.
Good idea. I also added a couple comments and simplified the return logic.
…ror. - Incorporated feedback and fixes
This is motivated to have a version that includes latest bblfsh/sdk#135
This fix was trivial to code, easy to explain but was a hell to debug.
When there was any error on the reader from the native driver, the driver stdin was not being consumed until the next newline. This caused that on the next request it would still trying to write to its stdout while the Go wrapper would try to read the new request into its stdin, hanging.
This also changes the read to be done by reading buffer-sized chunks into a slice so this should also fix the "buffer size exceeded" problem.
The driver images should be rebuild once this is merged since it's a pretty annoying bug.