Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

layerStore.Diff(): don't add an unnecessary buffer #1087

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

nalind
Copy link
Member

@nalind nalind commented Dec 9, 2021

Don't ReadAll() from a Reader to create a buffer and then create another Reader to read from that buffer.

@rhatdan
Copy link
Member

rhatdan commented Dec 9, 2021

LGTM, if this works. Is this something we should test against buildah before we merge?

@TomSweeneyRedHat
Copy link
Member

Changes LGTM, tests do not. A test with Buildah would be good.

Don't ReadAll() from a Reader to create a buffer and then create another
Reader to read from that buffer.

Don't close a file and a decompressor that we're using to read the
file's contents when we we may still need to read from them after the
current function returns.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@nalind
Copy link
Member Author

nalind commented Dec 13, 2021

We were potentially closing the underlying file and the pgzip decompression filter before the JSON decoder had finished reading from the decompression filter, and that appears to have created a hang when we stopped buffering up all of the file's contents at the first step. The JSON decoder and pgzip decompressor are both advertised as potentially reading more information than they strictly require if there's some available, so I'm guessing that a partial read left one or the other waiting for more information that would never come.

@rhatdan
Copy link
Member

rhatdan commented Dec 13, 2021

LGTM

@rhatdan rhatdan merged commit 00b3393 into containers:main Dec 13, 2021
@nalind nalind deleted the redundant-readall branch December 16, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants