Skip to content

fix pusher concurrent close channel#7473

Merged
samuelkarp merged 1 commit intocontainerd:mainfrom
lengrongfu:fix/push_concurrent_close_channel
Oct 18, 2022
Merged

fix pusher concurrent close channel#7473
samuelkarp merged 1 commit intocontainerd:mainfrom
lengrongfu:fix/push_concurrent_close_channel

Conversation

@lengrongfu
Copy link
Copy Markdown
Contributor

@lengrongfu lengrongfu commented Oct 4, 2022 •
edited
Loading

goroutine1:

containerd/remotes/docker/pusher.go

Line 290 in 8abee9c

pushw.Close()

goroutine2:

containerd/remotes/docker/pusher_test.go

Line 152 in 8abee9c

defer cw.Close()

Use sync.Once to ensure that close(pw.pipeC) is executed only once.

issue: #7471

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Oct 4, 2022

Hi @lengrongfu. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lengrongfu lengrongfu force-pushed the fix/push_concurrent_close_channel branch from 7b9e3b6 to 0791663 Compare October 4, 2022 14:39
dcantah
dcantah reviewed Oct 5, 2022
View reviewed changes
Comment thread remotes/docker/pusher.go Outdated
@lengrongfu lengrongfu force-pushed the fix/push_concurrent_close_channel branch 2 times, most recently from da3da4e to 7318653 Compare October 5, 2022 03:43
akhilerm
akhilerm reviewed Oct 5, 2022
View reviewed changes
Comment thread remotes/docker/pusher.go Outdated
@lengrongfu lengrongfu force-pushed the fix/push_concurrent_close_channel branch from 7318653 to 63a7d8a Compare October 8, 2022 03:28
@lengrongfu
Copy link
Copy Markdown
Contributor Author

lengrongfu commented Oct 9, 2022

@akhilerm please take a look.

akhilerm
akhilerm reviewed Oct 10, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@lengrongfu have a question on the change in the test. The change in Close() looks good.

Comment thread remotes/docker/pusher.go
Comment thread remotes/docker/pusher_test.go
@lengrongfu
Copy link
Copy Markdown
Contributor Author

lengrongfu commented Oct 13, 2022

@dcantah Hi，can you help review this PR? thanks！

dcantah
dcantah approved these changes Oct 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

Was on my list 😆. LGTM

@lengrongfu
Copy link
Copy Markdown
Contributor Author

lengrongfu commented Oct 18, 2022

/ok-to-test

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Oct 18, 2022

@lengrongfu: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@samuelkarp
Copy link
Copy Markdown
Member

samuelkarp commented Oct 18, 2022

/ok-to-test

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Oct 18, 2022

@samuelkarp k8s jobs are ✅

@samuelkarp samuelkarp merged commit 9d0e1f5 into containerd:main Oct 18, 2022
@akhilerm
Copy link
Copy Markdown
Member

akhilerm commented Oct 19, 2022

I think this should be cherry-picked to 1.6 and 1.5

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Oct 19, 2022

@akhilerm 1.6: #7562

1.5 has some merge conflicts, but I can possibly look later today/this week. Feel free to cp it if you have time

@estesp estesp added cherry-pick/1.5.x cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch labels Oct 19, 2022
@akhilerm
Copy link
Copy Markdown
Member

akhilerm commented Oct 20, 2022

@dcantah The merge conflicts are because of the changes in this PR. Once this gets merged, the cherry-pick should be not have any conflicts. I will take care of the cherry-pick once the other PR is merged.

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Oct 20, 2022

Perfect! I'm not going to have time tomorrow so thanks a bunch

@akhilerm akhilerm mentioned this pull request Nov 3, 2022
Merged
@akhilerm
Copy link
Copy Markdown
Member

akhilerm commented Nov 5, 2022

cherry-picked this PR to #7479

@estesp estesp added cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch and removed cherry-pick/1.5.x labels Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Iceber Iceber Iceber left review comments

@samuelkarp samuelkarp samuelkarp approved these changes

@akhilerm akhilerm akhilerm approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

No one assigned

Labels

cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lengrongfu @k8s-ci-robot @samuelkarp @dcantah @akhilerm @Iceber @estesp