Repository: knative/pkg
Pull request #2574 (2022-08-17 21:00:47)

fix double close of channel

knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2574#" title="Author self-approved">dprotaso</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[network/OWNERS](https://github.com/knative/pkg/blob/main/network/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dprotaso: /assign @nader-ziada 
knative-prow[bot]: @dprotaso: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_pkg_main | a391829345b9be89553e3dba9d2f5506743f586e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2574/unit-tests_pkg_main/1560008771923611648) | true | `/test unit-tests_pkg_main`

[Your PR dashboard](https://prow.knative.dev/pr).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2574?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2574](https://codecov.io/gh/knative/pkg/pull/2574?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (a391829) into [main](https://codecov.io/gh/knative/pkg/commit/f78a006943077275297d759a65eb628355b1c750?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (f78a006) will **increase** coverage by `0.10%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main    #2574      +/-   ##
==========================================
+ Coverage   81.27%   81.37%   +0.10%     
==========================================
  Files         162      162              
  Lines        9737     9736       -1     
==========================================
+ Hits         7914     7923       +9     
+ Misses       1585     1577       -8     
+ Partials      238      236       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2574?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [network/handlers/drain.go](https://codecov.io/gh/knative/pkg/pull/2574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-bmV0d29yay9oYW5kbGVycy9kcmFpbi5nbw==) | `97.14% <100.00%> (+6.57%)` | :arrow_up: |
| [controller/controller.go](https://codecov.io/gh/knative/pkg/pull/2574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-Y29udHJvbGxlci9jb250cm9sbGVyLmdv) | `88.78% <0.00%> (+0.71%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

dprotaso: /retest - dial flake
knative-prow[bot]: @dprotaso: The `/retest` command does not accept any targets.
The following commands are available to trigger required jobs:
* `/test build-tests_pkg_main`
* `/test integration-tests_pkg_main`
* `/test unit-tests_pkg_main`

Use `/test all` to run all jobs.


<details>

In response to [this](https://github.com/knative/pkg/pull/2574#issuecomment-1218511508):

>/retest - dial flake


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
nader-ziada: /retest
nader-ziada: /lgtm 

Patch:
diff --git a/network/handlers/drain.go b/network/handlers/drain.go
index 5252016f0..5eee2ea42 100644
--- a/network/handlers/drain.go
+++ b/network/handlers/drain.go
@@ -73,8 +73,11 @@ type Drainer struct {
 	// timer is used to orchestrate the drain.
 	timer timer
 
-	// used to synchronize callers of Drain and Reset
-	ch chan struct{}
+	// used to synchronize callers of Drain
+	drainCh chan struct{}
+
+	// used to synchronize Drain and Reset
+	resetCh chan struct{}
 
 	// HealthCheckUAPrefixes are the additional user agent prefixes that trigger the
 	// drainer's health check
@@ -118,8 +121,8 @@ func (d *Drainer) Drain() {
 	ch := func() chan struct{} {
 		d.Lock()
 		defer d.Unlock()
-		if d.ch != nil {
-			return d.ch
+		if d.drainCh != nil {
+			return d.drainCh
 		}
 
 		if d.QuietPeriod <= 0 {
@@ -127,32 +130,26 @@ func (d *Drainer) Drain() {
 		}
 
 		timer := newTimer(d.QuietPeriod)
-		ch := make(chan struct{})
+		drainCh := make(chan struct{})
+		resetCh := make(chan struct{})
 
 		go func() {
 			select {
-			case <-ch:
-				// closed by reset
+			case <-resetCh:
 			case <-timer.tickChan():
-				close(ch)
 			}
+			close(drainCh)
 		}()
 
-		d.ch = ch
+		d.drainCh = drainCh
+		d.resetCh = resetCh
 		d.timer = timer
-		return ch
+		return drainCh
 	}()
 
 	<-ch
 }
 
-func drainTimer(tc <-chan time.Time) {
-	select {
-	case <-tc:
-	default:
-	}
-}
-
 // isHealthcheckRequest validates if the request has a user agent that is for healthcheck
 func (d *Drainer) isHealthCheckRequest(r *http.Request) bool {
 	if network.IsKubeletProbe(r) {
@@ -175,16 +172,16 @@ func (d *Drainer) Reset() {
 	defer d.Unlock()
 
 	if d.timer != nil {
-		if d.timer.Stop() {
-			d.timer = nil
-		} else {
-			drainTimer(d.timer.tickChan())
-		}
+		d.timer.Stop()
+		d.timer = nil
 	}
 
-	if d.ch != nil {
-		close(d.ch)
-		d.ch = nil
+	if d.resetCh != nil {
+		close(d.resetCh)
+		d.resetCh = nil
+	}
+	if d.drainCh != nil {
+		d.drainCh = nil
 	}
 }
 


Test patch:
diff --git a/network/handlers/drain_test.go b/network/handlers/drain_test.go
index 22c55706b..082b8100d 100644
--- a/network/handlers/drain_test.go
+++ b/network/handlers/drain_test.go
@@ -542,4 +542,7 @@ func TestReset(t *testing.T) {
 	if diff > 50*time.Millisecond {
 		t.Error("expected to drain to wait QuietPeriod time after reset")
 	}
+
+	// Calling reset after a drain should succeed
+	d.Reset()
 }