Fix panic in multiListener

[denpeshkov]

If Close() is called concurrently, both goroutines could pass the select’s default case before either of them executes close(ml.stopCh), resulting in the channel being closed twice

/kind bug

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: denpeshkov / name: Denis Peshkov (14892a7)

[k8s-ci-robot]

Welcome @denpeshkov!

It looks like this is your first PR to kubernetes/utils 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/utils has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[liggitt]

go ahead and squash commits once the comment is addressed, then this lgtm

[liggitt]

lgtm, ack from network reviewer would be great as well (@aojea?)

[aojea]

/lgtm
/approve

Thanks , good one

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, denpeshkov

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:

• OWNERS [aojea]

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

[ash2k]

Wouldn't it be better to use a sync.Once to ensure all callers see the effects of a concurrent Close if Close returns for them?

i.e. a caller may rely on a listener actually being closed vs being in the process of being closed when Close returns. The current behavior does not match the doc since it might not wait for the goroutines to exit:

utils/net/multi_listen.go

Lines 151 to 152 in 0af2bda

	// Close implements net.Listener. It will close all sub-listeners and wait for
	// the go-routines to exit.

I can open a PR if the above makes sense.

[aojea]

I do not completely follow, but the behavior that we have now IIUIC is:

• You can only can call Close() once, the second time it returns an error
• The first Close() call wait for the goroutines to exit

Is your point about the second call to Close() , if the fist Close() didn't finish yet, it should also wait?

[ash2k]

Yes. Today, unless you know you are the first/only caller, you cannot rely on Close() to block until all goroutines finish. It might give you an error and then you have no way to wait.

[ash2k]

Also, I'm not sure I understand why returning an error on double close is important but at the same time we discard all the actual errors from listeners/connections that we close.

[aojea]

Also, I'm not sure I understand why returning an error on double close is important but at the same time we discard all the actual errors from listeners/connections that we close.

I honestly can;t remember, and do not have much time these days to dig deeper, I assume that if a listener is close we do not care about bubbling up the error since it is already close and that is the intention.

The best next step should be to ensure we match the behavior of the existing standard listener , since the goal was to make a compatible one for dual stack use cases, see the golang original issue golang/go#9334 that is still open

[ash2k]

Opened #331.