Fix handling for json-file io.UnexpectedEOF#42104
Conversation
|
s/io.UnepxectedEOF/io.UnexpectedEOF/ |
|
@tonistiigi @tiborvass ptal |
243d349 to
ccdc6f5
Compare
thaJeztah
changed the title
|
@thaJeztah That's correct. |
There was a problem hiding this comment.
iiuc the issue in here is not the resetting d.rdr or not (what looks more correct to me in the old version) but that we use incompatible Reader where after it returns io.EOF we can read again and it may return more data then. This does not follow the io.Reader interface that stdlib defines and therefore the utility functions behavior gets wrong. More specifically io.MultiReader caches io.EOF https://github.com/golang/go/blob/414fa8c35e7c2f65e2c767d6db2f25791e53b5c1/src/io/multi.go#L41 . So in the previous code if the MultiReader has already returned EOF once and now is added as a second reader to a new MultiReader it never gets called again and new MultiReader returns EOF without making any new Read calls to the original d.rdr.
There was a problem hiding this comment.
Added a 2nd commit per your suggestion to replace io.MultiReader with a custom implementation that does not cache EOF's so that we can keep the full reader (with the buffered data) in d.rdr.
When the multireader hits EOF, we will always get EOF from it, so we cannot store the multrireader fro later error handling, only for the decoder. Thanks @tobiasstadler for pointing this error out. Signed-off-by: Brian Goff <[email protected]>
ccdc6f5 to
4be98a3
Compare
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
LGTM
SyntaxError handler seems to require combinedReader as well but that can be done separately as not directly related. Although I wonder if it might be broken because it could not have been made to work before because of the multireader issue.
|
Yeah there's no test for syntax reader and I think it never actually fixed anything. |
There was a problem hiding this comment.
| func comineReaders(pre, rdr io.Reader) io.Reader { | |
| func combineReaders(pre, rdr io.Reader) io.Reader { |
Tonis mentioned that we can run into issues if there is more error handling added here. This adds a custom reader implementation which is like io.MultiReader except it does not cache EOF's. What got us into trouble in the first place is `io.MultiReader` will always return EOF once it has received an EOF, however the error handling that we are going for is to recover from an EOF because the underlying file is a file which can have more data added to it after EOF. Signed-off-by: Brian Goff <[email protected]>
4518545 to
5a664dc
Compare
|
CI was green, and only typo was fixed in last push |
This flaky inspect test :( |
|
When will this be included in a release? |
|
It's backported to the 20.10 release branch through #42174, and will be included in the 20.10.6 patch release |
|
Thanks for following up, do you know when 20.10.6 will be released? |
|
Should be available soon (if all goes well, this week) (famous last words 😂 ) |
|
It seems to be fixed in docker engine 20.10.6 https://docs.docker.com/engine/release-notes/#20106 |
|
almost 2am here. updating docker with this patch. Praying for this fix to solve my issues :) |
|
@BenasPaulikas docker 20.10.6 and 20.10.7 have already shipped with this patch, so updating to the latest patch release should bring this patch. |
|
I'm still seeing this issue on 20.10.17, and so are many people on #41820 (comment) |
|
Same here, docker 20.10.10 |
|
I might be wrong here. But there was package that was using json to get docker output or something. They changed to use streams (or something simmilar) and issue was gone. + It saved so much CPU power |
This PR was to address an issue where docker tried to read the JSON, got an EOF, and reused the reader (which would continue to return an EOF error). Note that the EOF error can be an actual issue with a corrupt file, in which case the |
I can confirm I have this problem again on multiple systems running docker. I would also suggest that this issue is reopened. I'm also running version: 20.10.17 |
|
Same issue here. All our Docker instances are spamming this error continuously. Version: 20.10.22 |
|
Same issue here with docker 20.10.18. Getting a couple of warnings every second (but it is not affecting my ability to process container logs) This triggered by using Loki's promtail agent to collect docker logs using the API (aka, "docker logs') via the service discovery plugin for docker: |
11 participants
Before this, we essentially end up doubling any buffered data from the
json decoder by storing it in both
d.decandd.rdr.Thanks @tobiasstadler for pointing this error out.
Fixes #41820