Tim Bates wrote:
You have some excitement ahead of you!
Here's one potential problem:
At this point we've just decided that free_handles is not empty.
But before the next line happens, another thread executes and steals the
last handle. The return value is nil.
In the empty case, there is a less serious problem:
Two threads can get to this point at the same time. Then they both
create a new handle, even though that might result in MAX_HANDLES+1
handles. So, _very_ gradually, the size of the pool grows.
By the same token, this code:
could cause too many handles to be deleted: it's possible for N threads
to be scheduled to check the condition, and then after all that checking
is done, each calls destroy_handle. So you end up with too few free
There is also a performance problem with code like:
Rather than wake up each second and check for an available handle, the
thread would be better off going to sleep indefinitely, and being
wakened when a handle is available. This would save context-switches at
a rate of twice per waiting thread per second. And it would reduce delay
from 0.5sec average to (probably) milliseconds.
On the positive side, this code won't ever give the same handle to two
threads, because #shift is atomic, as far as ruby threads are concerned.
The construct that would probably apply best here is the Queue in
thread.rb. It would replace free_handles (just create 20 handles and
put them on the queue, use #pop and #push to wait for and to release
handles). It would solve the performance problem as described (a thread
goes to sleep waiting for a handle, and is woken by another thread when
there is a handle available in the queue). It would also avoid the race
condition inherent in checking empty? and then shifting.
However, it won't help with the MAX_FREE_HANDLES logic that you've
designed. You would probably have to use Thread.critical for that, so
that you can check the size of the queue and be certain that it isn't
changing while you decide to destroy a handle or not.