threadly/threadly — pull request #55 by jentfoo (2014-01-28 18:51:04+00:00)
Title: This is an attempt to resolve #54.

Sending a pull request so @lwahlmeier can give his opinion. The changes to the synchronization for getExistingWorker and makeNewWorker cause a lot of unit test changes in order to verify the locking.

We solve the issue by tracking how many tasks are waiting for an existing worker. If there are any waiting, we just start workers (as long as we remain under our max pool size). That way it puts the burden on the thread increasing the max pool size, instead of in the normal run time path of how we consume workers.

Because of this change I removed the synchronized block for makeNewWorker and getExistingWorker. The lock is REQUIRED, I removed the synchronization because synchronizing at that point is too late. This is demonstrated in the following situation:
* Pre condition: All workers are busy handling tasks and so none are available
* Thread 1 increases max pool size, thread 1 locks workersLock to check if any are waiting for a worker. No one is blocking waiting for a worker, so no workers are started even though none are available. This is to expect that if future tasks need a worker, they will make it on their own.
* Thread 2 called to get existing worker. It blocks waiting to lock while thread 1 finishes.
* Thread 2 wakes up after Thread 1 releases the lock...but no workers are available since none were started. So we block for the maxWaitTime. In order to prevent this we had to lock before we made the decision to call getExistingWorker (ie verified the core size < max pool size, and we should not create a worker instead)

Because it does not make sense to lock here any more, but in practice we must lock, I created a "StrictPriorityScheduledExecutor". This class can verify assumptions in test that don't need to be done always in runtime. It does not make sense to verify these things always because the only way to violate this rule is to extend the class.