search

nawazish-github / go-worker-pool

A managed go-routine worker pool which accepts task and schedules them to be executed in its managed pool of go-routine workers.
0 stars 0 forks source link

Some feedback #1

Open abhijpes opened 1 year ago

abhijpes commented 1 year ago

Couldn't start a code review on this github repo, hence adding a few feedbacks here:

  1. https://github.com/nawazish-github/go-worker-pool/blob/main/pool/pool_impl.go#L40

The order in which the runnable will be assigned to the goroutine (aka worker) is non-deterministic / depends on the order in which goroutines are scheduled, which will result in imbalance of load (not good as we scale and go to higher request rate).

  1. https://github.com/nawazish-github/go-worker-pool/blob/main/pool/pool_impl.go#L15

"numWrkrs" is not being tracked properly. When a worker gets shutdown, numWrkrs should decrement.

  1. https://github.com/nawazish-github/go-worker-pool/blob/main/pool/pool_impl.go#L49-L50

The exit channel is an unbuffered channel with "numWrkrs" receivers listening on this channel. Pushing onto an unbuffered channel is a blocking call unless there is at least 1 receiver ready to receive. What if all workers are busy? We will be unblocked only as soon as the first worker gets done with its job, followed by the second worker, followed by 3rd...

So, time that this loop will take to run = Summation of duration of all tasks current running across all workers. Solution - Just close() the runnableCh which will be broadcasted to all workers.

  1. Lastly, I would disagree with referring to goroutines are User space threads. Goroutines are scheduled on user threads by the Go scheduler. For more details: https://www.ardanlabs.com/blog/2018/08/scheduling-in-go-part1.html
abhijpes commented 1 year ago
  1. More of a continuation of point 3 - https://github.com/nawazish-github/go-worker-pool/blob/main/pool/pool_impl.go#L62-L65

This is incorrect as you cannot determine whether the worker will consume from runnableCh or exit channel.

If both - runnableCh and exit has messages on the channel, the runtime is free to choose to pick up from any of the 2 channels. If this worker has been told to exit, it should (at best) finish off this task and exit immediately.