On Mon, Nov 25, 2013 at 1:27 PM, Jim Jagielski <[email protected]> wrote:
> > On Nov 24, 2013, at 7:18 PM, Jeff Trawick <[email protected]> wrote: > > > On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <[email protected]> > 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/
