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

Replace WaitGroup using semaphore instead #3

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

godoylucase
Copy link
Owner

No description provided.

@godoylucase godoylucase force-pushed the refactor/use-semaphore branch 3 times, most recently from 6ed4a98 to 47016ba Compare July 4, 2021 17:33
@peterbourgon
Copy link

A semaphore is usually used to constrain the parallelism of a concurrent operation to less than its theoretical maximum concurrency. For example, if you need to make a bunch of GET requests, but only want 3 in-flight at once, you might do

semaphore := make(chan struct{}, 3)
for _, url := range urls {
    go func(url string) {
        semaphore <- struct{}{} // acquire
        defer func() { <-semaphore }() // release
        resp, err := http.Get(url)
        // ...
    }(url)
}

Semaphores just have those two operations — acquire, and release. I think you misinterpret the Wiki definition when it mentions wait — that doesn't map to an operation, but rather describes what happens when a goroutine tries to acquire on a semaphore that's already at capacity.

What you've created here isn't precisely a semaphore. You're just using a channel as a counter! It works. But I bet there's a much easier way to do what you're trying to do, with a lot less code :)

@godoylucase
Copy link
Owner Author

@peterbourgon thanks for your comments, what you said it's right. It is a semaphore hybrid wrapped into a struct. I will try to pull apart the wait method by segregating interfaces. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants