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.

Sign up for GitHub

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

Jump to bottom

fix pusher concurrent close channel #7473

Merged
merged 1 commit into from Oct 18, 2022
Merged

fix pusher concurrent close channel #7473

merged 1 commit into from Oct 18, 2022

Conversation

lengrongfu
Copy link
Contributor

@lengrongfu lengrongfu commented Oct 4, 2022 •
edited

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

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.

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
remotes/docker/pusher.go Outdated Show resolved Hide resolved
@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
remotes/docker/pusher.go Outdated Show resolved Hide resolved
@lengrongfu
fix pusher concurrent close channel
63a7d8a 
Signed-off-by: rongfu.leng <rongfu.leng@daocloud.io>
@lengrongfu lengrongfu force-pushed the fix/push_concurrent_close_channel branch from 7318653 to 63a7d8a Compare October 8, 2022 03:28
@lengrongfu
Copy link
Contributor Author

@akhilerm please take a look.

akhilerm
akhilerm reviewed Oct 10, 2022
View reviewed changes
Copy link
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.

remotes/docker/pusher.go Show resolved Hide resolved
remotes/docker/pusher_test.go Show resolved Hide resolved
@lengrongfu
Copy link
Contributor Author

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

dcantah
dcantah approved these changes Oct 13, 2022
View reviewed changes
Copy link
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
Contributor Author

/ok-to-test

@k8s-ci-robot
Copy link

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

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
Member

/ok-to-test

@dcantah
Copy link
Member

dcantah commented Oct 18, 2022

@samuelkarp k8s jobs are ✅

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

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

@dcantah
Copy link
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 Change to be cherry picked to release/1.5 branch cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch labels Oct 19, 2022
@akhilerm
Copy link
Member

@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
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
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 Change to be cherry picked to release/1.5 branch 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.

None yet
7 participants
@lengrongfu @k8s-ci-robot @samuelkarp @dcantah @akhilerm @Iceber @estesp