Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add utils.Subprocesses to replace sync.WaitGroup #53

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Conversation

topliceanu
Copy link
Collaborator

No description provided.

@topliceanu topliceanu requested a review from jmank88 June 20, 2022 14:32
Comment on lines +20 to +46
type Subprocesses struct {
wg sync.WaitGroup
}

// Wait blocks until all function calls from the Go method have returned.
func (s *Subprocesses) Wait() {
s.wg.Wait()
}

// Go calls the given function in a new goroutine.
func (s *Subprocesses) Go(f func()) {
s.wg.Add(1)
go func() {
defer s.wg.Done()
f()
}()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems useful 👍 , but WDYT about keeping it super simple, like a basic extension with the same name that embeds or uses sync.WaitGroup as the underlying type?

Suggested change
type Subprocesses struct {
wg sync.WaitGroup
}
// Wait blocks until all function calls from the Go method have returned.
func (s *Subprocesses) Wait() {
s.wg.Wait()
}
// Go calls the given function in a new goroutine.
func (s *Subprocesses) Go(f func()) {
s.wg.Add(1)
go func() {
defer s.wg.Done()
f()
}()
}
type WaitGroup struct {
sync.WaitGroup
}
type WaitGroup sync.WaitGroup
// Go calls the given function in a new goroutine.
func (wg *WaitGroup) Go(f func()) {
wg.Add(1)
go func() {
defer wg.Done()
f()
}()
}

Either way, you can drop the explicit Wait method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. The main idea of Subprocesses is that users CAN'T Add() more or less than they Done().
This happens to me a lot and it's a pain to debug. With your solution, users can still get into this situation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, IMHO the docs should say more about how to use this properly without assuming any knowledge of WaitGroup. For example, a naive user that calls Go() after Wait() would cause a race. Alternatively, if this was moved to package monitoring, then I think it's fine as-is - I'm just overly concerned about keeping pkg/utils API super clean/stable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I'll add documentation for pkg/utils#Subprocesses.

@topliceanu
Copy link
Collaborator Author

@jmank88 I had to rebase over main

@topliceanu topliceanu merged commit 4fe1fc1 into main Jun 22, 2022
@topliceanu topliceanu deleted the add-subprocesses branch June 22, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants