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

Limit concurrent protoc instances #290

Merged
merged 4 commits into from
Aug 31, 2021
Merged

Conversation

ryantking
Copy link
Contributor

No description provided.

@ryantking ryantking requested a review from ilackarms August 31, 2021 15:42
ilackarms
ilackarms previously approved these changes Aug 31, 2021
@ryantking ryantking added the work in progress This pr is still being worked on label Aug 31, 2021
@ryantking ryantking removed the work in progress This pr is still being worked on label Aug 31, 2021
@soloio-bulldozer soloio-bulldozer bot merged commit 2c57f51 into master Aug 31, 2021
@soloio-bulldozer soloio-bulldozer bot deleted the limit-concurrency branch August 31, 2021 20:02
if err != nil {
return nil, eris.Wrapf(err, "invalid value for MAX_CONCURRENT_PROTOCS: %s", s)
}
sem = make(chan struct{}, maxProtocs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this chan need to be closed at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original PR author here: No, closing go channels is only required when the business logic requires that you indicate that all data has been sent through the channel. Since channels are not backed by a file descriptor like a network connection or file on disk, closing them is purely semantic. Once the open channel goes out of scope it will be garbage collected like any other variable.

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.

3 participants