edicl/hunchentoot — pull request #37 by lemoinem (2012-08-22 15:23:59+00:00)
Title: Fix taskmaster max accept count behavior

# Notes:

1. I have another pull request regarding fd exhaustion when the connection is prematurely closed (on either side), however, it is not merge-compatible with this one, so I prefer to wait considering this one first. I found the current issue while looking for the source of fd exhaustion.

2. If you think it is useful, I may also prepare a patch to fix the apparent discrepancies between the LW implementation and the generic one (regarding create-request-handler-thread, which does not seem to be impervious to fix/error-while-creating-thread-does-not-decrement-request-counter, nor to Jul 06, 2009 (Handle conditions signaled during worker process creation) fix.

# Notes on the Pull request itself:

1. This fix includes two modifications, merged together in the first commit. If you prefer to have one commit for each modification, simply say so and I will split the commit in two.

2. I also tried to port my fix to the LW implementation, however, since both implementations are not following the exact same execution path, I may have made an error or overlook something that would be LW specific. The LW backport is therefore in a separated commit.

Here is a copy of the first commit message (plus formatting and an additional proof-reading pass), explaining in details the issues and the fixes I applied (or considered).

Several issues had to be considered to end up with the correct behavior:

## Threading behavior:

`taskmaster-max-accept-count` was not respected because `handle-incoming-connection`
was executed in the main acceptor thread. As soon as `taskmaster-request-count`
reached `taskmaster-max-thread-count`, the thread was put to sleep. The new
connections were therefore not accepted, eventually reaching the acceptor listen
backlog limit and being silently dropped by the kernel (instead of returning an
HTTP 503 answer).

### FIX: _The handler thread is now spawned earlier_

That way the main acceptor thread is not blocked.

## Counter behavior:

Moreover, even with the thread spawned earlier, the behavior was not valid: in
`handle-incoming-connection`, when `taskmaster-request-count` reached
`taskmaster-max-thread-count`, the current handling thread was put to sleep,
without incrementing `taskmaster-request-count` first. Thus
`taskmaster-request-count` would never reach `taskmaster-max-accept-count` and never
trigger the `too-many-taskmaster-requests behavior` (and sending an HTTP 503
answer).

The acceptor would therefore keep accepting new connections until eventually
exhausting the available amount of file descriptors (fds) allowed for each
process (given an heavy enough load).

### SUGGESTED FIX: _incrementing `taskmaster-request-count` earlier_

This solution is not acceptable. Although it would actually fix the fd
exhaustion and adequately trigger the `too-many-taskmaster-requests` behavior, we
would expose ourselves to the following scenario:

Let's take `taskmaster-max-thread-count` = 100 and `taskmaster-max-accept-count` =
120 (which are the default values). Let's assume 140 requests are received by
the server quickly enough for the last request to be accepted before the first
processing is complete, but not quickly enough for the number of
not-yet-accepted connections to reach the acceptor listen backlog (iow, all
requests are given to `handle-incoming-connection` more or less
simultaneously):

The first 100 requests will start being processed right away
(`taskmaster-request-count` < `taskmaster-max-thread-count`), the next 20 requests
will be put into a waiting state (`taskmaster-max-thread-count` <=
`taskmaster-request-count` < `taskmaster-max-accept-count`) and the last 20 requests
will be rejected with an HTTP 503 answer (`taskmaster-request-count` >=
`taskmaster-max-accept-count`).

`taskmaster-request-count` will now have the value 120. Once the first 10 requests
processing are completed, it will be decreased to 110 (with 20 threads still in
waiting state and 90 requests being processed). Although `note-free-connection`
will have been called (10 times), the condition in `wait-for-free-connection`
(`taskmaster-request-count` < `taskmaster-max-thread-count`) will never be satisfied
and no thread will be woken up. Until 11 more requests processing are completed
and `taskmaster-request-count` finaly reaches 99, at which point, one thread will
be woken up. For a grand total of 80 requests being processed while 20 others
are still sleeping (!). The requests will therefore be put to sleep much longer
than they would need.

Even worse, if `taskmaster-max-accept-count` > 2*`taskmaster-max-thread-count`,
there will always be too many sleeping threads for `taskmaster-request-count` to
ever reach `taskmaster-max-thread-count` and hunchentoot will eventually reject
any new request without processing any old request currently waiting.

### FIX: _using 2 separated counters, one for accepted connections, one for processed connections_

We now use two different counters. While `taskmaster-request-count` still count
the number of requests being processed (and we didn't change its
increment/decrement strategy, we created another counter,
`taskmaster-accept-count`, which is counting the number of requests that have been
currently accepted (iow, the REAL number of thread currently spawned, either
being processed or in a waiting state). This counter is incremented by
`handle-incoming-connection%` as soon as we fork from the acceptor thread and is
decremented before the end of thread, either in `send-service-unavailable-reply`
or when the number `taskmaster-request-count` is decremented.

## Acceptor listen backlog race condition

Of course, if the load is so heavy that hunchentoot receives requests faster
than it can send HTTP 503 answers, the acceptor listen backlog will eventually
be reached and connections will be silently closed.

To fix this issue would mean to set an extremely high listen backlog (or disable
it altogether). However, that would delay the moment by which hunchentoot fall
back to a stable state after the heavy load spike because it would have to send
an HTTP 503 answer for each and every connection waiting to be accepted. This
solution is therefore perceived as increasing the probability of hunchentoot
failing to process queries in case of consecutives spikes and slowing down the
recovery process.

This behavior is thus deemed as acceptable, if not wanted, in order to guarantee
a better reliability under consecutive spike/heavy load.