Skip to content

fix a race in json logger reader#17537

Merged
LK4D4 merged 1 commit intomoby:masterfrom
mountkin:fix-logger-race
Oct 31, 2015
Merged

fix a race in json logger reader#17537
LK4D4 merged 1 commit intomoby:masterfrom
mountkin:fix-logger-race

Conversation

@mountkin
Copy link
Contributor

The json decoder starts to decode immediately a inotify event is
received.
But at the time the inotify event is trigged, the json log
entry might haven't been fully written to the disk.
In this case the decoder will return an "io.UnexpectedEOF" error, but
there is still data remaining in the decoder's buffer. And the data
should be passed to the decoder when the next inotify event is
triggered.

fixes https://jenkins.dockerproject.org/job/Windows-PRs/16236/console and https://jenkins.dockerproject.org/job/Docker-PRs/18950/console
Signed-off-by: Shijiang Wei [email protected]

@mountkin
Copy link
Contributor Author

The json decoder starts to decode immediately an inotify event is
received.
But at the time the inotify event is trigged, the json log
entry might haven't been fully written to the disk.
In this case the decoder will return an "io.UnexpectedEOF" error, but
there is still data remaining in the decoder's buffer. And the data
should be passed to the decoder when the next inotify event is
triggered.

Signed-off-by: Shijiang Wei <[email protected]>
@cpuguy83
Copy link
Member

Oh nice catch!
Did you by chance see this error?

@mountkin
Copy link
Contributor Author

@cpuguy83 I can 100% reproduce the error by wrapping the body of TestRunSlowStdoutConsumer or TestLogsFollowSlowStdoutConsumer in a loop and run the integration test.

@cpuguy83
Copy link
Member

That's really awesome!

@cpuguy83
Copy link
Member

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Oct 31, 2015

LGTM

LK4D4 added a commit that referenced this pull request Oct 31, 2015
fix a race in json logger reader
@LK4D4 LK4D4 merged commit ed6e3cf into moby:master Oct 31, 2015
@mountkin mountkin deleted the fix-logger-race branch October 31, 2015 07:28
@mountkin
Copy link
Contributor Author

mountkin commented Nov 2, 2015

hmm... still seeing the io.ErrUnexpectedEOF error after 10 (maxJSONDecodeRetry) retries.
Shall we increase the maxJSONDecodeRetry to 15 or 20? Or is there a better approach to handle the io.ErrUnexpectedEOF in json decoder? @cpuguy83 @LK4D4

@mountkin
Copy link
Contributor Author

mountkin commented Nov 2, 2015

Still failing even if I increased the maxJSONDecodeRetry to 20.
I removed the retry counter when io.ErrUnexpectedEOF occurs and the test passed.

        if err == io.ErrUnexpectedEOF {
          reader := io.MultiReader(dec.Buffered(), f)
          dec = json.NewDecoder(reader)
          continue
        }

But I'm wondering if there are chances that it falls into a dead loop 😓 To make it safer I think we can change maxJSONDecodeRetry to 20000 or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants