channel neither has values in nor be closed . #2547
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
Hi @wayilau. Thanks for your PR. I'm waiting for a kubernetes-incubator or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -35,7 +36,7 @@ func RunServer(opts *ServiceCatalogServerOptions, stopCh <-chan struct{}) error | |||
| if stopCh == nil { | |||
| /* the caller of RunServer should generate the stop channel | |||
| if there is a need to stop the API server */ | |||
| stopCh = make(chan struct{}) | |||
| stopCh = server.SetupSignalHandler() | |||
There was a problem hiding this comment.
should check the callers of this and make sure they're supplying a channel.
My guess is that there is the cli/cmd , and tests.
We should probably crash if this isn't set, but I haven't looked close enough to know.
There was a problem hiding this comment.
i will have a look about this. thank you for your review @MHBauer
There was a problem hiding this comment.
Have you looked into this? what happens when it isn't set?
There was a problem hiding this comment.
sorry, i was busy these days. i Will have a look it today or tomorrow.
There was a problem hiding this comment.
@jberkhahn the caller give a channel actually, which created by server.SetupSignalHandler().
but if stopCh == nil, just make one can not be closed at all. so i give the code above.
There was a problem hiding this comment.
from that description, I think this should panic and crash with an informative message.
There was a problem hiding this comment.
@MHBauer i modify the code to "panic", also give a message when the stop channel was not set. Please help review it when you are idle.
|
/retest |
Add Tests for this case.
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
do-not-merge/hold
| if there is a need to stop the API server */ | ||
| stopCh = make(chan struct{}) | ||
| /* whe RespectsStopCh: true and the stop channel is nil, error returned*/ | ||
| return fmt.Errorf("%v", "the stop channel can not be nil") |
There was a problem hiding this comment.
An error is good enough, but I still think this should be a panic. it's unreachable in normal case. If someone is going to library-ize this code and use it in their own apiserver, they need to know to set it.
There was a problem hiding this comment.
yes, i will change the code.
There was a problem hiding this comment.
@MHBauer i've changed the code comment as it is.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MHBauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@MHBauer @jberkhahn |
|
I'm fine with this |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
* channel neither has values in nor be closed . * panic api server when stop channel not set * Return Error when the stop channel is nil and RespectsStopCh=true Add Tests for this case.
This PR is a
What this PR does / why we need it:
channel never closed when create a new one by "make", but also has no values in it.