On Mon, 15 Jun 2009, Mindaugas Kavaliauskas wrote:
Hi,
> I know that thread count could be changed between HB_MUTEXWAITERSCOUNT()
> and actual HB_THREADSTART(), but I do not care a lot about it. But it would
> be bad if this race condition can cause return garbage on some systems. In
> this case it need to be protected by mutex. It could be renamed to
> __MUTEX*() of couse, but I feel, that sometimes it could be useful to have
> mutex information function. I found two things useful: waiters count and
> queue length. Maybe we can create some more general mutex protected
> function like:
> HB_MUTEXQUEUEINFO( hMutex, [ @nWaitersCount ], [ @nQueueLength ] )
> or another name similar name.
The race condition in this case is not hardware problem (we do not need
additional synchronization mechanism to protect pMutex->waiters at least
on machines for which Harbour can be ported) but logical one.
Just simply the returned value does not give any precise information
which can be used in some algorithms which need strict number of waiters.
For programmer it's only approximated number of waiters. Of course such
information can also be usable but only if used algorithm does not need
strict value.
Due to missing synchronization between taking value and checked event
it can return bigger or smaller value then the one which programmer expect.
It can be well seen in your code:
DO WHILE .T.
IF (nI := socket_select({Self:hListen},,, 1000)) > 0
hSocket := socket_accept(Self:hListen)
nWaiters := hb_mutexWaitersCount(Self:hmtxQueue) /* (*) */
IF nWaiters < 2 .AND. LEN(aThreads) < THREAD_COUNT_MAX
/*
We need two threads in worst case. If first thread becomes a sessioned
thread, the second one will continue to serve sessionless requests for
the same connection. We create two threads here to avoid free thread
count check (and aThreads variable sync) in ProcessRequest().
*/
AADD(aThreads, hb_threadStart(@ProcessConnection(), Self))
AADD(aThreads, hb_threadStart(@ProcessConnection(), Self))
ENDIF
hb_mutexNotify(Self:hmtxQueue, {hSocket, ""})
ENDIF
ENDDO
Please note that in fact it's not guarantied that you will create such
two threads for each connection. This code can create more or less threads.
I.e. it's possible that only two threads will be created for 100 connections.
Just simply two new threads created for 1-st connection are stopped inside
ProcessConnection() at:
hb_mutexSubscribe(oServer:hmtxQueue,, @aI)
and not woken up immediately after:
hb_mutexNotify(Self:hmtxQueue, {hSocket, ""})
then main thread receives next 99 connections and executes
hb_mutexNotify(Self:hmtxQueue, {hSocket, ""})
for each of them. It may happen because it's not guarantied that OS wakes
up waiting thread immediately after notification. I know that the chance
is very small but users used to forget about such things and create code
which needs some strict time condition. On one computer such code can work
for a year without any problems on some others it may crash immediately.
In this case it's also possible that some threads will never serve incoming
connection because threads created for previous one will have finish his
job and took new one before them. It will be enough that OS will not execute
new thread immediately after hb_threadStart() or freeze them just before
hb_mutexSubscribe().
I haven't analyzed the whole code yet but I guess that in this case nothing
very wrong will happen. Just simply the server may not be as efficient as
expected due to not perfect approximation returned by hb_mutexWaitersCount().
The function name is less important here and using '__' is even more
confusing because it suggests that it's some internal function. It's not
the core of problem. hb_mutexWaitersCount() cannot be used as atomic
function to synchronize any code because there is always race condition
which effects returned value which can be bigger (mutex is notified but
threads not woken up yet) or smaller (some threads are frozen before
incrementing pMutex->waiters). It's important that programmers understand
it. That's all.
> Przemek, I think your opinion is most valuable here. Or maybe you propose a
> completely different problem solution. The problem is worker threads count
> management: if too many workers are free, it would be good to decrease
> workers count; if job queue length is too big, it would be good to start
> new worker threads.
In this case you will have better approximation if you also use the size
of message queue and calculate the difference between pMutex->waiters
and hb_arrayLen( pMutex->events ) (accessing pMutex->events should be
covered by lock) and proposed by you HB_MUTEXQUEUEINFO() gives such
information. But it's also only approximation and can be used only to
tune some parameters not as direct synchronization mechanism.
To help you in this case I will have to carefully look at the uhttpd2
code. When I'll try to make it when I'll find some spare time.
best regards,
Przemek
_______________________________________________
Harbour mailing list
Harbour@harbour-project.org
http://lists.harbour-project.org/mailman/listinfo/harbour
