search

gammazero / workerpool

Concurrency limiting goroutine pool
MIT License
1.33k stars 138 forks source link

add RunningCount() method to pool #53

Closed costela closed 3 years ago

costela commented 3 years ago

This returns the number of currently running jobs, analogously to how WaitingQueueSize() returns the size of the waiting queue.

To be able to access the new running pool attribute, I took the liberty of refactoring worker() and startWorker() as methods of WorkerPool (as opposed to functions). Since all member accesses are either protected by atomic or channels, this should not be a problem, but please let me know if you think I missed something.

Naming is of course up for bikeshedding :wink:

gammazero commented 3 years ago

Thank you very much for your interest in this project and contributing to it. I like this change and it all looks good, but I have been holding off doing anything with it because it is something that can be also be provided by a wrapper function... that increments a counter on entry, then calls the wrapped function, and decrements the counter on exit, without having to be built into the workerpool implementation.

Such a wrapper can also provide entry and exit logging, tracking of execution times, histogram of functions executed, controlling the pace at which tasks are executed (see pacer), etc.

Generally I would not want to build these functionalities into the workerpool, because they are not really something the worker pool needs to do, and IMO should be provided by the application than needs them, or by a utility library. I could see a use for a set of wrappers that does many of the things above, including keeping track of functions that are currently running. These wrappers could be applied to any function and used with any goroutine or worker pool, not just this one.

costela commented 3 years ago

hi @gammazero, thanks for the feedback!

That sounds like a totally valid reason not to include this change! At the time it felt like this sort of simple accounting wouldn't bloat the lib too much, but your suggested alternative is easy enough to make it redundant.

So I'll just go ahead and close it then! :+1: