allow holder to close/open/close without panic on closing closed channel

[travisturner]

Overview

This PR uses a sync.Once to prevent closing the Holder.opened channel after it has been closed. The downside to this solution is that there's no support for waiting on Holder.opened on the second Open() event, but since this is a non-exported channel, that's currently not functionality we need internally.

It also re-initializes the Holder.closing channel inside the Open() method. This allows multiple calls to Close() without panic. Here again, since Holder.closing is not exported, it doesn't cause a problem to initialize it after creation of Holder.

Fixes #1685

Pull request checklist

• I have read the contributing guide.
• I have agreed to the Contributor License Agreement.
• I have updated the documentation.
• I have resolved any merge conflicts.
• I have included tests that cover my changes.
• All new and existing tests pass.

Code review checklist

This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

• Ensure that any changes to external docs have been included in this pull request.
• If the changes require that minor/major versions need to be updated, tag the PR appropriately.
• Ensure the new code is properly commented and follows Idiomatic Go.
• Check that tests have been written and that they cover the new functionality.
• Run tests and ensure they pass.
• Build and run the code, performing any applicable integration testing.

[jaffee]

what if Close reset the opened chan? I think that would avoid races and the need for the once

[travisturner]

I thought about that, but I'm not sure what would happen if Close() was called asynchronously, at the same time, or immediately after, calling Open(). That could result in a process that is blocking on opened never releasing the block. I believe in that case, Open() would end up closing the channel that was reset in the Close() method. Maybe it doesn't matter since these are all non-exported channels, but I felt like that was more confusing. And we could always implement it that way later if there was a case where we needed to block on opened more than once.

[jaffee]

I think not calling Open and Close concurrently and calling Open before Close is a reasonable API restriction.

My concern with the current implementation is that we might expose the ability to check if a holder is opened (e.g. via a WaitForOpen method or similar, but forget that that will only work the first time.

[travisturner]

That's ok with me. I don't think the asynchronous Open/Close is really a concern at this point. I updated it to reset both of those channels.