Skip to content

pkg/virtual/workspaces/registry: fix panic sending to a closed channel #1889

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
Merged
merged 1 commit into from
Sep 6, 2022
Merged

pkg/virtual/workspaces/registry: fix panic sending to a closed channel #1889

merged 1 commit into from
Sep 6, 2022

Conversation

davidfestal
Copy link
Member

Summary

This PR should fix a panic in the workspaces virtual workspace.

Related issue(s)

A panic is occuring in the workspaces virtual workspace, forcing the virtual workspace command to restart:

panic: send on closed channel

goroutine 431 [running]:
github.com/kcp-dev/kcp/pkg/virtual/workspaces/registry.withProjection.ResultChan.func1()
/workspace/pkg/virtual/workspaces/registry/rest.go:442 +0xf7
created by github.com/kcp-dev/kcp/pkg/virtual/workspaces/registry.withProjection.ResultChan
/workspace/pkg/virtual/workspaces/registry/rest.go:431 +0xc5

@openshift-ci openshift-ci bot requested review from shawn-hurley and sttts September 5, 2022 12:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from davidfestal by writing /assign @davidfestal in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@davidfestal
Copy link
Member Author

/retest

@s-urbaniak
Copy link
Contributor

s-urbaniak commented Sep 6, 2022 •
edited
Loading

To stop the bleeding this seems reasonable however ResultChan() and Watch() is currently not covered by unit testing. As a follow-up I suggest we create a dedicated unit test asserting the channel state machine (given that unit tests are covered with -race).

@davidfestal @s-urbaniak
pkg/virtual/workspaces/registry: fix panic sending to a closed channel
bf418bd 
Currently, the projection watch interface closes its result channel in Stop().
This can cause a panic when the goroutine started in ResultChan() still tries
to send data to the result channel which can be closed.

This fixes it by making sure that the goroutine started in ResultChan()
itself closes the channel once the delegate watch is done.
@s-urbaniak s-urbaniak force-pushed the fix-panic-in-virtual-workspace branch from 1fd7d35 to bf418bd Compare September 6, 2022 12:32
@s-urbaniak s-urbaniak changed the title Fix a panic in the virtual workspace pkg/virtual/workspaces/registry: fix panic sending to a closed channel Sep 6, 2022
@davidfestal
Copy link
Member Author

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 6, 2022

@davidfestal: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@csams
Copy link
Contributor

csams commented Sep 6, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2022
@davidfestal davidfestal merged commit 59a9bd4 into main Sep 6, 2022
@ncdc
Copy link
Member

ncdc commented Sep 6, 2022

/cherry-pick release-0.8

@openshift-cherrypick-robot
Copy link

@ncdc: new pull request created: #1897

In response to this:

/cherry-pick release-0.8

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Sep 6, 2022
Merged
@s-urbaniak s-urbaniak deleted the fix-panic-in-virtual-workspace branch September 6, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@sttts sttts sttts left review comments

@aojea aojea aojea left review comments

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

Assignees

@csams csams

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@davidfestal @s-urbaniak @csams @ncdc @openshift-cherrypick-robot @sttts @aojea