Reduxed with channel semaphore

[l3x]

Proposed changes include:

• Using a buffered channel
• Using a groupSemaphre to wait for goroutines to finish
• Closing the membersChannel when we're done with it (have members list populated)
• Eliminating the loop that counts seenMembers.

main.go

FROM

	members := make([]*thresholdgroup.Member, 0, beaconConfig.GroupSize)
	memberChannel := make(chan *thresholdgroup.Member)
	for i := 0; i < beaconConfig.GroupSize; i++ {
		go func(i int) {
			member, err := relay.ExecuteDKG(chainCounter, channel, beaconConfig.GroupSize, beaconConfig.Threshold)
			if err != nil {
				fmt.Fprintf(
					os.Stderr,
					"[member:%v] Failed to run DKG: [%s] (index %d).",
					member.BlsID.GetHexString(),
					err,
					i)
				memberChannel <- nil
				return
			}

			memberChannel <- member
		}(i)
	}

	seenMembers := 0
	for member := range memberChannel {
		seenMembers++
		if member != nil {
			members = append(members, member)
			if len(members) == beaconConfig.GroupSize {
				break
			}
		}

		if seenMembers == beaconConfig.GroupSize {
			break
		}
	}

TO

	members := make([]*thresholdgroup.Member, 0, beaconConfig.GroupSize)
	memberChannel := make(chan *thresholdgroup.Member, beaconConfig.GroupSize)
	type empty struct{}
	groupSemaphore := make(chan empty, beaconConfig.GroupSize);
	for i := 0; i < beaconConfig.GroupSize; i++ {
		go func(i int) {
			member, err := relay.ExecuteDKG(chainCounter, channel, beaconConfig.GroupSize, beaconConfig.Threshold)
			if err != nil {
				fmt.Fprintf(
					os.Stderr,
					"[member:%v] Failed to run DKG: [%s] (index %d).",
					member.BlsID.GetHexString(),
					err,
					i)
				memberChannel <- nil
				return
			}

			memberChannel <- member
			groupSemaphore <- empty{};
		}(i)
	}
	// Wait for goroutines to finish
	for i := 0; i < beaconConfig.GroupSize; i++ { <-groupSemaphore }
	/// Populate members list
	for member := range memberChannel {
		members = append(members, member)
		if len(members) == beaconConfig.GroupSize { close(memberChannel) }
	}

[mhluongo]

No reason to include code differences in the PR description- we can see it in the diff

[rargulati]

Hey @l3x

Using a buffered channel and closing the members channel are great ideas/ nice optimizations.

Specifically regarding Using a groupSemaphre to wait for goroutines to finish, in Go (and for safety), you'll want to use one of the sync primitives when trying to coordinate goroutines. In this case, it's best to use sync.WaitGroup https://golang.org/pkg/sync/#WaitGroup. This will avoid using two channels to race against each other / control the flow. The code would look like:

	members := make([]*thresholdgroup.Member, 0, beaconConfig.GroupSize)
	memberChannel := make(chan *thresholdgroup.Member, beaconConfig.GroupSize)
	var wg sync.WaitGroup

	for i := 0; i < beaconConfig.GroupSize; i++ {
		wg.Add(1)
		go func(i int) {
			defer wg.Done()
			member, err := relay.ExecuteDKG(chainCounter, channel, beaconConfig.GroupSize, beaconConfig.Threshold)
			if err != nil {
				fmt.Fprintf(
					os.Stderr,
					"[member:%v] Failed to run DKG: [%s] (index %d).",
					member.BlsID.GetHexString(),
					err,
					i)
				memberChannel <- nil // do we need this?
				return
			}

			memberChannel <- member
		}(i)
	}
	// Wait for goroutines to finish
	wg.Wait()

       // we don't need the conditional as Wait enforces and synchronizes the goroutines
	close(memberChannel)

	/// Populate members list
	for member := range memberChannel {
		members = append(members, member)
	}

This would help you remove a few things, specifically:

// Wait for goroutines to finish
for i := 0; i < beaconConfig.GroupSize; i++ { <-groupSemaphore }

thereby truly satisfying Eliminating the loop that counts seenMembers, replacing all that with a simple wg.Wait(), and:

if len(members) == beaconConfig.GroupSize { close(memberChannel) }

Also, another fun trick, rather than using var empty struct{}, you could have set groupSemaphor's chan type to struct and then filled it with groupSemaphor <- struct{}{}.

Side note, from the sync package I also really like Cond, which provides a rendezvous point for goroutines. The atomic package also has some nice primitives for concurrent access. More importantly, these are battle tested, performant, and safe!

As for merging this in/final thoughts, I'll leave it to @Shadowfiend

[l3x]

I agree! Using that pattern is better...

var wg sync.WaitGroup
// loop
    wg.Add(1)
    		go func() {
    			defer wg.Done()
    			...
    			
// end loop    			
wg.Wait()

... It can be used in more scenarios. Howevever, with the one memberChannel I'm not sure there'd be a chance for race condition using my old-school semaphore method?

Either way, I think we have a win!

Formatting suggestion

Does this look more compact and readable...

           fmt.Fprintf(os.Stderr,
				    "[member:%v] Failed to run DKG: [%s] (index %d).",
					member.BlsID.GetHexString(), err, i)

... than this?

           fmt.Fprintf(
					os.Stderr,
					"[member:%v] Failed to run DKG: [%s] (index %d).",
					member.BlsID.GetHexString(),
					err,
					i)

p.s. Thanks for the groupSemaphor <- struct{}{} tip!

[Shadowfiend]

Nice discussion y'all, learned a bit more about available primitives and such!

A few more “process-y” notes on this PR (for all participants):

• As @mhluongo notes, including the code that is already in the PR diff is an anti-pattern. It makes the description longer without adding information, since the PR diff provides a more readable, commentable version of the same information.
• When posting a comment that includes code, minimize the code to just the bits that you're trying to talk about. Any additional code is noise and makes it harder to understand your point. For example:

	var wg sync.WaitGroup

	for i := 0; i < beaconConfig.GroupSize; i++ {
		wg.Add(1)
		go func(i int) {
			defer wg.Done()
			…
		}(i)
	}
	// Wait for goroutines to finish
	wg.Wait()

       // we don't need the conditional as Wait enforces and synchronizes the goroutines
	close(memberChannel)
       …

• Let's avoid using Markdown headings in PR comments. Use them sparingly in PR descriptions as well, and if you do feel the need, please use level 3/4 or higher (####), rather than levels 1 and 2. Headings attract the eye very heavily, and make it a lot harder to read through many comments (or skim through descriptions). Comments should be short enough that they don't need headings, for the most part.
• The lines in question are in the original PR, so these changes would best have been left as questions/remarks with reasoning on the original PR, for the author to implement (with an advantage that they will probably learn more along the way; bonus points, I'm the author in this case :D). I probably threw this off by including a link to the reduced diff in the PR's description, so my apologies.
• There are several proposed changes here, but no explicit reasoning for any of them:
  • As line comments on the original PR, these would have taken the form (e.g.) “Should we use a buffered channel here? That would allow ....” No code needed, just a request for a change + some context for why; the author can make the change and learn something in the process.
  • As commits, they should probably have been 2-3 commits, each one with a description that explained why the change was being made, rather than having 1 commit with a description that explains what changes were made.

Again, good discussion; we'll narrow in on improving our process practices so no worries.

With all of that said, if we'd made these requests on the original PR, I would probably have declined to implement them (as I did when Raghav mentioned something similar in the original Guard Tower PR) because I expect this main function to be lit on fire within 2 weeks at most 😞 It's quick, it's dirty, and it's short-lived; unless it's actually broken (which I don't think it is?), it's not really worth spending much time with it. If you've got concerns about the existing stuff actually being broken, please share them on the original PR; for now, I'll close this one.

Does this look more compact and readable...

More compact, less readable, IMO. It's easier to have a solid rule on how to break things into lines (multiple lines = one arg per line) than it is to vary it (multiple lines = however many args per line the current line width admits; this makes determining all of a function's arguments at a glance more complicated). As a general rule, I prefer readability/skimmability to compactness. Making code review a core part of our process also emphasizes this preference, since you are guaranteed to have at least one person trying to read through your code who isn't you.

[Shadowfiend]

I wanted to expand a touch on some of the implications of the “let the author work on their own PR” principle… I thought about it some more yesterday and this morning and realized I provided some poor advice yesterday. My key conclusion I want to get at: opening a PR against an open PR is almost never the best thing to do. Of course… I totally suggested it as a solution yesterday, so what gives?

In general, opening a PR against an open PR should be a last resort. To start, comments in a PR should rarely be code, they should be English requests + reasoning for why those requests are a good idea. They might feature a code fragment or two to clarify what you mean, but their primary purpose is communicating gaps/problems and why you think they are gaps/problems. The author of the PR is assumed to be a good developer; if they have clarification questions, they'll ask them, and if they have trouble implementing your English requests in code, they should ask as well. Now, if you go back and forth a few times in a PR, but you feel like you're not getting anywhere, then and only then it can be helpful to fire up a branch so you can show, in code, what you mean. At this point, it still may not make sense to open a PR. You can link directly to the commit you made on your branch from the author's PR, from an existing comment thread, and say “this is more what I had in mind”. Your code in this case isn't meant for inclusion, but for education.[1]

This circles back to the idea that code review is a learning opportunity for the author. If, as a reviewer, you spot an issue and call it out, you've spotted something the author didn't. That means there's a teaching opportunity for the reviewer, and a learning opportunity for the author. Implementing the change for the author (a) requires you to flip into their work (inefficient for you) and (b) reduces their opportunity to fully investigate and interact with the suggestion you're making.

Asking a question in a line or PR comment ("Should we…?”) and justifying it ("so that…") has a few implications:

• It requires you to formalize your own thinking about why you think something is better.
• It allows for the possibility that the author considered this option but didn't pursue it (and gives them the opportunity to explain).
• It allows for the possibility that your justification is out of scope, as well as the possibility that this is a mistake (“doing this would allow X, but X isn't a goal; should it be?”).

Now, as usual, these aren't ironclad rules. There are times when it makes sense to drop a suggestion, and you don't fully know why you prefer it. In general, though, I expect this to be rare; really, I'd consider it a case of “don't break the rules until you know them”: don't let yourself make a suggestion unless you can explain it for a while. If someone makes a suggestion but doesn't explain it, ask them to justify. It helps us all understand each other a little bit better, too!

---

[1] This ignores the possibility of having a pair programming call where you explain your thoughts/show what you mean. Just don't forget to summarize the conclusions of that call in a comment or commit message. The author can commit the final changes and include the reasoning that was reached, for example. Or, if the conclusion is that the change isn't quite right, the reason for that can be posted on the comment thread.