Added debug to apparent stop channel called early. #687
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako 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
cncf-cla: yes
|
/test pull-apiserver-network-proxy-test-master |
k8s-ci-robot
added
size/S
|
/assign @tallclair |
| select { | ||
| case <-a.stopCh: | ||
| return | ||
| default: | ||
| } | ||
| timeout := time.Now().Add(15 * time.Second) | ||
| eConn.conn.SetReadDeadline(timeout) |
There was a problem hiding this comment.
I think what we want to do here is close the connection if stopch is closed. Something like this?
go func() {
<-a.stopCh
eConn.conn.Close()
}The documentation on Close says it terminates any blocking read or write operations.
There was a problem hiding this comment.
This function has a defer cleanup() call on line 533. When we return it should get called. That cleanup makes a protected call to
apiserver-network-proxy/pkg/agent/client.go
Line 386 in c1a1e09
There was a problem hiding this comment.
Is it a problem to call close() multiple times? I wasn't suggesting that we were leaving the connection open, but rather calling close should be a way to quickly react to the stop channel, rather than polling for it every 15 seconds, which is what I understood your change to be doing?
There was a problem hiding this comment.
I don't believe close in golang is re-entrant. In fact I think the reason we had to protect the clean up with callOnce() in that code path is specifically because close causes a panic if called twice.
No description provided.