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

use unbuffered channels #128

Closed
wants to merge 1 commit into from
Closed

use unbuffered channels #128

wants to merge 1 commit into from

Conversation

malisetti
Copy link

@malisetti malisetti commented Sep 8, 2020

In #126 the output channel does not have to a buffered channel and having buffer length equal to length of total processes is possibly not the best buffer length because there may not be all go related processes. imo, this change makes FindAll function easier to understand in terms of the flow.

@tklauser
Copy link
Collaborator

tklauser commented Sep 8, 2020

Does this fix an issue in the worker concurreny which #126 didn't address? Why is using unbuffered channels better? Please state in the PR description and/or commit message why this change is necessary, not just what it does.

@malisetti
Copy link
Author

@tklauser Updated the description of the PR.

@tklauser
Copy link
Collaborator

tklauser commented Sep 8, 2020

... and having buffer length equal to length of total processes is possibly not the best buffer length because there may not be all go related processes

I don't see why it would matter that not all of the processes are Go processes. The idea is to account for all currently running processes on the system and then let concurrencyLimit goroutines work to determine whether they are Go processes.

this change makes FindAll function easier to understand in terms of the flow.

I think the current solution is fairly easy to understand and I don't think this PR makes understanding the control flow easier. Unless there is a bug in the current solution, I'd prefer to leave it that way.

@malisetti malisetti closed this Sep 8, 2020
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.

2 participants