Skip to content

archive: refactor race fixing logic to prevent potential deadlock#39920

Open
tiborvass wants to merge 2 commits intomoby:masterfrom
tiborvass:archive-fix-potential-deadlock
Open

archive: refactor race fixing logic to prevent potential deadlock#39920
tiborvass wants to merge 2 commits intomoby:masterfrom
tiborvass:archive-fix-potential-deadlock

Conversation

@tiborvass
Copy link
Contributor

@tiborvass tiborvass commented Sep 12, 2019

cpuguy83
cpuguy83 requested changes Sep 12, 2019
View reviewed changes
@tiborvass tiborvass force-pushed the archive-fix-potential-deadlock branch 2 times, most recently from fcc61bb to 70492a5 Compare September 12, 2019 22:12
@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 12, 2019

Added a test for the deadlock, thanks @tonistiigi

cpuguy83
cpuguy83 approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 12, 2019

The test is rightfully failing for me when only taking the first commit (without the fix)

@stbenjam stbenjam mentioned this pull request Sep 18, 2019
Closed
@tiborvass tiborvass mentioned this pull request Sep 19, 2019
Merged
@thaJeztah
Copy link
Member

thaJeztah commented Sep 23, 2019

@tiborvass failures look related;

https://ci.docker.com/public/job/moby/job/PR-39920/3/execution/node/207/log/

00:03:03.520  failed to register layer: Error processing tar file(read |0: file already closed):

https://ci.docker.com/public/job/moby/job/PR-39920/3/execution/node/212/log/

00:09:48.551  error loading frozen images: Error processing tar file(read |0: file already closed):

https://ci.docker.com/public/job/moby/job/PR-39920/3/execution/node/217/log/

00:10:32.212  === Failed
00:10:32.212  === FAIL: pkg/archive TestPigz (0.01s)
00:10:32.212      archive_test.go:1336: Tested whether Pigz is used, as it installed
00:10:32.212      archive_test.go:1339: assertion failed: *os.File (*reflect.rtype) != *io.PipeReader (*reflect.rtype)
00:10:32.212  
00:10:32.212

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 23, 2019
@thaJeztah thaJeztah mentioned this pull request Sep 23, 2019
Closed
@thaJeztah thaJeztah added the kind/bugfix PR's that fix bugs label Oct 10, 2019
@thaJeztah
Copy link
Member

thaJeztah commented Oct 13, 2019

ping @tiborvass - what's the status on this one?

@thaJeztah
Copy link
Member

thaJeztah commented Oct 24, 2019

ping @tiborvass

@andrewhsu
Copy link
Contributor

andrewhsu commented Oct 31, 2019

@tiborvass when you get a chance, rebase this to get the latest tests run on this change

@thaJeztah
Copy link
Member

thaJeztah commented Nov 22, 2019

ping @tiborvass

@thaJeztah
Copy link
Member

thaJeztah commented Jan 17, 2020

ping @tiborvass PTAL

1 similar comment
@thaJeztah
Copy link
Member

thaJeztah commented Feb 22, 2020

ping @tiborvass PTAL

@tiborvass
archive: add TestCmdStreamDeadlock
81878b3 
The following currently fails

```
GO111MODUEL=off go test -run TestCmdStreamDeadlock ./pkg/archive
```

and is fixed by the next commit.

Signed-off-by: Tibor Vass <teabee89@gmail.com>
@tiborvass
archive: refactor race fixing logic to prevent potential deadlock
a47c090 
Signed-off-by: Tibor Vass <teabee89@gmail.com>
@tiborvass tiborvass force-pushed the archive-fix-potential-deadlock branch from 70492a5 to a47c090 Compare December 1, 2022 01:37
@tiborvass
Copy link
Contributor Author

tiborvass commented Dec 1, 2022

@thaJeztah Rebased

@thaJeztah
Copy link
Member

thaJeztah commented Dec 1, 2022

Hm... something's either broken (I've seen this error on another PRs, so might not be related), or there was a glitch that we hit. Let me kick CI again.

Error: No such image: emptyfs
Loaded image: emptyfs:latest
Running integration-test (iteration 1)
Running /go/src/github.com/docker/docker/integration/build (amd64.integration.build) flags=-test.v -test.timeout=5m  -test.coverprofile=/go/src/github.com/docker/docker/bundles/test-integration/amd64-integration-build-coverage.out
error loading frozen images: read |0: file already closed

=== Failed
=== FAIL: amd64.integration.build  (0.00s)
error loading frozen images: read |0: file already closed

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

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

kind/bugfix PR's that fix bugs process/cherry-pick status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tiborvass @thaJeztah @andrewhsu @cpuguy83