Hi, It's holiday season and things may be moving slower than it usually is. The patch have been available for a while, and it fixes the port code for Solaris which I really like to get it in.
It could be somewhat controversial for the atomic code used to defer the port_associate in apr_pollset_add in order to pass the testpoll code, which is the needed fix if we consider the semantic of apr_pollset_poll should be giving accumulated events; Otherwise, we should revise the test/testpoll.c. With that said, I break the patch into two, the first port_1203-1.diff fix bugs of: 1. Return APR_SUCCESS if the poll removed from pollset is in the add_ring, which means the poll had been added but is current disassociated because an event. 2. In apr_pollcb_poll, call apr_pollcb_add to ensure port_associate. And the port_1203-2.diff is the fix to pass testpoll given current situation, which is less a concern as I consider in most apps, apr_pollset_poll is called in a loop. Any comments are welcomed. Cheers, Henry
Index: poll/unix/port.c =================================================================== --- poll/unix/port.c (revision 600793) +++ poll/unix/port.c (working copy) @@ -192,6 +192,7 @@ pfd_elem_t *ep; apr_status_t rv = APR_SUCCESS; int res; + int err; pollset_lock_rings(); @@ -205,6 +206,7 @@ res = port_dissociate(pollset->port_fd, PORT_SOURCE_FD, fd); if (res < 0) { + err = errno; rv = APR_NOTFOUND; } @@ -233,6 +235,9 @@ APR_RING_REMOVE(ep, link); APR_RING_INSERT_TAIL(&(pollset->dead_ring), ep, pfd_elem_t, link); + if (ENOENT == err) { + rv = APR_SUCCESS; + } break; } } @@ -464,6 +469,7 @@ if (rv) { return rv; } + rv = apr_pollcb_add(pollcb, pollfd); } }
--- poll/unix/port_1.c Mon Dec 3 21:37:14 2007 +++ poll/unix/port.c Mon Dec 3 21:37:41 2007 @@ -15,6 +15,7 @@ */ #include "apr_arch_poll_private.h" +#include "apr_atomic.h" #ifdef POLLSET_USES_PORT @@ -80,6 +81,8 @@ /* A ring of pollfd_t where rings that have been _remove'd but might still be inside a _poll */ APR_RING_HEAD(pfd_dead_ring_t, pfd_elem_t) dead_ring; + /* number of threads in poll */ + volatile apr_uint32_t waiting; }; static apr_status_t backend_cleanup(void *p_) @@ -110,6 +113,7 @@ return APR_ENOTIMPL; } #endif + (*pollset)->waiting = 0; (*pollset)->nelts = 0; (*pollset)->nalloc = size; (*pollset)->flags = flags; @@ -168,16 +172,22 @@ fd = descriptor->desc.f->filedes; } - res = port_associate(pollset->port_fd, PORT_SOURCE_FD, fd, - get_event(descriptor->reqevents), (void *)elem); + if (apr_atomic_read32(&pollset->waiting)) { + res = port_associate(pollset->port_fd, PORT_SOURCE_FD, fd, + get_event(descriptor->reqevents), (void *)elem); - if (res < 0) { - rv = APR_ENOMEM; - APR_RING_INSERT_TAIL(&(pollset->free_ring), elem, pfd_elem_t, link); - } + if (res < 0) { + rv = APR_ENOMEM; + APR_RING_INSERT_TAIL(&(pollset->free_ring), elem, pfd_elem_t, link); + } + else { + pollset->nelts++; + APR_RING_INSERT_TAIL(&(pollset->query_ring), elem, pfd_elem_t, link); + } + } else { pollset->nelts++; - APR_RING_INSERT_TAIL(&(pollset->query_ring), elem, pfd_elem_t, link); + APR_RING_INSERT_TAIL(&(pollset->add_ring), elem, pfd_elem_t, link); } pollset_unlock_rings(); @@ -273,6 +283,8 @@ pollset_lock_rings(); + apr_atomic_inc32(&pollset->waiting); + while (!APR_RING_EMPTY(&(pollset->add_ring), pfd_elem_t, link)) { ep = APR_RING_FIRST(&(pollset->add_ring)); APR_RING_REMOVE(ep, link); @@ -296,6 +308,9 @@ ret = port_getn(pollset->port_fd, pollset->port_set, pollset->nalloc, &nget, tvptr); + /* decrease the waiting ASAP to reduce the window for calling + port_associate within apr_pollset_add() */ + apr_atomic_dec32(&pollset->waiting); (*num) = nget; if (ret == -1) {