Hi,
Attached is a patch fix the testpoll issues found on Solaris. It
combines the previous fix for removing a poll.
This patch basically minimize the timing window to make a
port_associate before someone is trying to retrieve events from the
port.
Cheers,
Henry
On 10/30/07, Henry Jen <[EMAIL PROTECTED]> wrote:
> On 10/30/07, Henry Jen <[EMAIL PROTECTED]> wrote:
> > On 10/29/07, William A. Rowe, Jr. <[EMAIL PROTECTED]> wrote:
> > > William A. Rowe, Jr. wrote:
> > >
> > > > * Solaris 10, testpoll.c:314 we are failing with only APR_POLLOUT when
> > > > we
> > > > had expected both APR_POLLIN and APR_POLLOUT to be signaled. Ideas?
> > >
> >
> > Seems like an issue with event port library, to associate with both
> > POLLIN and POLLOUT event does not work on Solaris. I had created a
> > simple test program as attached, and I noted that if we change the
> > associate to POLLIN only, we will get the POLLIN event.
> >
> > Disable detection of port_create from configure.in to force using poll
> > on Solaris pass the test.
> >
> > I am forwarding this to OpenSolaris developers and hopefully will get
> > some explanation.
> >
> >
>
> After some more investigation and read into the man page more carefully,
> I found the following explains the issue. The following discussion is
> based on the test code attached with previous email.
>
> Quoted from the man page of port_associate:
>
> > > Objects of type PORT_SOURCE_FD are file descriptors. The
> > > event types for PORT_SOURCE_FD objects are described in
> > > poll(2). At most one event notification will be generated
> > > per associated file descriptor. For example, if a file
> > > descriptor is associated with a port for the POLLRDNORM
> > > event and data is available on the file descriptor at the
> > > time the port_associate() function is called, an event is
> > > immediately sent to the port.
>
> This explains the first event only has POLLOUT flagged, as sendto is
> called after port_associate. By switching the order, we get an event
> with POLLIN and POLLOUT both flagged.
>
> > > If data is not yet available,
> > > one event is sent to the port when data first becomes avail-
> > > able.
> > >
> > > When an event for a PORT_SOURCE_FD object is retrieved, the
> > > object no longer has an association with the port. The
> > > event can be processed without the possibility that another
> > > thread can retrieve a subsequent event for the same object.
> > > After processing of the file descriptor is completed, the
> > > port_associate() function can be called to reassociate the
> > > object with the port.
>
> This indicates another call to port_associate is needed within the while
> loop to continue receive events interested. By adding port_associate
> into the while loop, the program will get the POLLIN event at the second
> attempt without changing the order of sendto call as mentioned above.
>
> So a hack to fix to testpoll.c is to call apr_pollset_add after send_msg.
>
> A better fix for Solaris, IMHO, is don't call port_associate in
> apr_pollset_add but leave it to apr_pollset_poll by putting the
> request into the add_ring. Attached patch fix get over this issue.
> However, testpoll still have other existing failures on my Solaris
> Developer Express system.
>
> testpoll : |Line 343: expected <0>, but saw <70015>
> |Line 523: expected <0>, but saw <70015>
> |Line 615: expected <0>, but saw <70015>
> FAILED 3 of 19
> Failed Tests Total Fail Failed %
> ===================================================
> testpoll 19 3 15.79%
>
> HTH,
> Henry
>
>
Index: poll/unix/port.c
===================================================================
--- poll/unix/port.c (revision 591399)
+++ poll/unix/port.c (working copy)
@@ -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();
@@ -192,6 +202,7 @@
pfd_elem_t *ep;
apr_status_t rv = APR_SUCCESS;
int res;
+ int err;
pollset_lock_rings();
@@ -205,6 +216,7 @@
res = port_dissociate(pollset->port_fd, PORT_SOURCE_FD, fd);
if (res < 0) {
+ err = errno;
rv = APR_NOTFOUND;
}
@@ -233,6 +245,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;
}
}
@@ -268,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);
@@ -291,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) {
@@ -464,6 +484,7 @@
if (rv) {
return rv;
}
+ rv = apr_pollcb_add(pollcb, pollfd);
}
}
