On Mon, Nov 25, 2013 at 1:27 PM, Jim Jagielski <j...@jagunet.com> wrote:
>
> On Nov 24, 2013, at 7:18 PM, Jeff Trawick <traw...@gmail.com> wrote:
>
> > On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <ylavic....@gmail.com>
> wrote:
> > Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check
> use :
> >
> >
> > prev_idlers = apr_atomic_add32((apr_uint32_t
> *)&(queue_info->idlers), -1);
> >
> > like ap_queue_info_wait_for_idler() does ?
> >
> > I think that's correct...
> >
> > What the code really wants is new_idlers, so edit that slightly to
> >
> > new_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers),
> -1) - 1;
> > if (new_idlers <= 0) {
> > ...
> > return APR_EAGAIN;
> > }
>
> It just wants to see if there are any idle ones... If there wasn't
> before the dec, then we need to return APR_EAGAIN. Recall that
> a 0 means "no idlers" and neg means 'this many blocking', and
> the logic is the same in both cases, minus the "wake up" scenario.
>
>
I think this is a safe progression from the original code, which was:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)&(queue_info->idlers));
if (prev_idlers <= 0) {
We know now that ALMOST every apr_atomic_dec32() returned the new idlers
value, and none returned the previous value, so fix the variable name to
reflect reality where it worked:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int new_idlers;
new_idlers = apr_atomic_dec32((apr_uint32_t *)&(queue_info->idlers));
if (new_idlers <= 0) {
You can't simply replace apr_atomic_dec32(foo) with apr_atomic_add32(foo,
-1) since the latter returns the old value instead of the new value, so you
have to subtract 1 from the return code of apr_atomic_dec32() to set
new_idlers to the same result as before.
--
Born in Roswell... married an alien...
http://emptyhammock.com/
