Calling init on worker pool twice no longer blocks

[jskelcy]

While working on a service that used two worker pool I accidentally fat fingered and called init twice on the same worker pool. As a result, the service hung forever because we try to blindly push to an already full channel.

This diff checks to see if there are already workers in the channel and if there are just returns. I could see this going either way where we panic/error or silently return. If someone feels strongly about one way or the other I would happy to make a change.

[jeromefroe]

Since the length of the worker channel can change as the pool is used would it be better to instead use an internal flag and only initialize the channel if the flag is not?

[jskelcy]

The work channel is not public, so it would have to change within this package, and there is no code to do that right now.

But in the future, I suppose it could change if some functionality was added. I can use a flag.

[jeromefroe]

I just meant that if one calls Go on the worker pool that will decrement the length of the channel until the worker is put back on the queue. So if there are any concurrent calls to Go between calls to Init you'll run into the same issue.

[cw9]

Not exactly sure what is this testing, if you remove the second Init() does the test still pass? Also should probably rename this test to something like TestInitOnInitializedWorkerPool

[jskelcy]

yeah this test is a little odd because init does not return anything. I just wanted to show that this no longer hangs forever. Maybe the name change will make it clear.

[xichen2020]

Is this really necessary by the way? I think in most cases the expectation is that Init is supposed to be called once. Perhaps we should just document it to make it clear that Init is only supposed to be called once instead of complicating the initialization logic.

[robskillington]

Either that or we can just get rid of the init method altogether, it'll be more expensive to create the worker pool when using the constructor but everyone usually just calls Init straight afterwards anyhow.

[cw9]

Feels a better test would be:

        p := NewWorkerPool(testWorkerPoolSize)
	p.Init()

	var wg sync.WaitGroup
	wg.Add(1)
	go func() {
	  p.Init()
	  wg.Done()
	}
	wg.Wait()

This way it's clear that it's avoiding the hanging, rather than checking the counts and stuff

[jskelcy]

Sure. I was thinking there should be some assert in the test but I suppose that is not necessarily true.

[jskelcy]

@xichen2020 totally valid that we just use documentation to handle this issue. As you can see in the description this only came up from a fat finger and it struck me as kind of odd. I think either documentation, error, or noops are all valid solutions, as long as we have a consistent design principle, but currently, we don't have any one of those solutions so we should pick one. Personally, I would prefer errors or noops over documentation but I think consistency is most important.

@robskillington yep. I was trying to avoid a backwards breaking change, but you are right that does seem like the common pattern.

[codecov]

Codecov Report

Merging #144 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage    77.1%   77.21%   +0.11%     
==========================================
  Files          79       79              
  Lines        3188     3191       +3     
==========================================
+ Hits         2458     2464       +6     
+ Misses        664      661       -3     
  Partials       66       66

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4129591...c87e398. Read the comment docs.