Repository: knative/pkg
Pull request #2575 (2022-08-18 00:35:12)

[release-1.7] fix double close of channel
This is an automated cherry-pick of #2574

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2575#" title="Author self-approved">knative-prow-robot</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign vagababov for approval by writing `/assign @vagababov` in a comment. For more information see:[The Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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

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

- **[network/OWNERS](https://github.com/knative/pkg/blob/release-1.7/network/OWNERS)**

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":["vagababov"]} -->
dprotaso: downstream fails because the branch doesn't exist

/override Unit Test (knative/serving)
/override Unit Test (knative/eventing) 
/override Unit Test (knative/operator) 
/override Unit Test (knative/client)
knative-prow[bot]: @dprotaso: Overrode contexts on behalf of dprotaso: Unit Test (knative/client), Unit Test (knative/eventing), Unit Test (knative/operator), Unit Test (knative/serving)

<details>

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

>downstream fails because the branch doesn't exist
>
>/override Unit Test (knative/serving)
>/override Unit Test (knative/eventing) 
>/override Unit Test (knative/operator) 
>/override Unit Test (knative/client)


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>
dprotaso: /lgtm
/approve
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2575#issuecomment-1218782011" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2575#" title="Author self-approved">knative-prow-robot</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/release-1.7/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":[]} -->

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()
 }