You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2006/03/14 22:19:31 UTC
testpoll.c:multi_event_pollset and 1.2.x
Does anyone have an objection to reverting the addition of the new
multi_event_pollset test on the 1.2.x branch? It's testing for
behavior that's never existed for a number of pollset back ends (at
least kqueue, potentially epoll as well), and nobody has jumped up to
make the test pass on those platforms in trunk, let alone get a fix
that can be backported.
For the curious, the problem is that kqueue doesn't consolidate the
events it returns, so you can get two separate events for the same
file descriptor, some read, some write, etc.
Given that we've always had this behavior in the 1.x releases and
nobody's complained about it, I'm willing to just say that the test is
bogus. If someone wants to rework the kqueue (and probably epoll)
impls to meet this new standard that's something that can be
discussed, but for now there's no real justification for us to
suddenly condemn their behavior.
Thoughts?
-garrett
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 3/16/06, Joe Orton <jo...@redhat.com> wrote:
> On Wed, Mar 15, 2006 at 01:13:58PM -0800, Garrett Rooney wrote:
> > On 3/15/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> ...
> > > > + else if (lrv == 2) {
> > > > + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> > > > + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> > > > + ABTS_PTR_EQUAL(tc, s[0], descs[1].desc.s);
> > > > + ABTS_PTR_EQUAL(tc, s[0], descs[1].client_data);
> > > > + ABTS_ASSERT(tc, "returned events incorrect",
> > > > + ((descs[0].rtnevents | descs[1].rtnevents)
> > > > + == (APR_POLLIN | APR_POLLOUT))
> > > > + && descs[0].rtnevents != descs[1].rtnevents);
> > >
> > > This fails because both rtnevents are actually APR_POLLIN | APR_POLLOUT.
>
> It fails as in the test fails, or it fails to catch the bug? It would
> need to be rtnevents ^ rtnevents to catch the bug?
The test failed. When I was writing that I still thought the problem
might be with the test, not the kqueue code.
> > Figured it out. See r386154 for the fix. Long standing bug in the
> > kqueue code. Will backport to 1.2.x real soon now.
>
> Nice work!
Thanks. One of those "I've been staring at this for weeks, how can
that possibly be the problem?" sort of things ;-)
-garrett
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Mar 15, 2006 at 01:13:58PM -0800, Garrett Rooney wrote:
> On 3/15/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
...
> > > + else if (lrv == 2) {
> > > + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> > > + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> > > + ABTS_PTR_EQUAL(tc, s[0], descs[1].desc.s);
> > > + ABTS_PTR_EQUAL(tc, s[0], descs[1].client_data);
> > > + ABTS_ASSERT(tc, "returned events incorrect",
> > > + ((descs[0].rtnevents | descs[1].rtnevents)
> > > + == (APR_POLLIN | APR_POLLOUT))
> > > + && descs[0].rtnevents != descs[1].rtnevents);
> >
> > This fails because both rtnevents are actually APR_POLLIN | APR_POLLOUT.
It fails as in the test fails, or it fails to catch the bug? It would
need to be rtnevents ^ rtnevents to catch the bug?
> Figured it out. See r386154 for the fix. Long standing bug in the
> kqueue code. Will backport to 1.2.x real soon now.
Nice work!
joe
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 3/15/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 3/15/06, Joe Orton <jo...@redhat.com> wrote:
>
> > Returning two separate descriptors should be valid behaviour, the test
> > just needs to be adjusted to handle that case; it'll still catch the bug
> > in the select() backend. Can you try this patch?
>
> Tried on FreeBSD:
>
> > Index: testpoll.c
> > ===================================================================
> > --- testpoll.c (revision 385518)
> > +++ testpoll.c (working copy)
> > @@ -309,10 +309,25 @@
> >
> > rv = apr_pollset_poll(pollset, 0, &lrv, &descs);
> > ABTS_INT_EQUAL(tc, 0, APR_STATUS_IS_TIMEUP(rv));
> > - ABTS_INT_EQUAL(tc, 1, lrv);
> > - ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> > - ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
> > - ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> > + if (lrv == 1) {
> > + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> > + ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
> > + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> > + }
> > + else if (lrv == 2) {
> > + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> > + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> > + ABTS_PTR_EQUAL(tc, s[0], descs[1].desc.s);
> > + ABTS_PTR_EQUAL(tc, s[0], descs[1].client_data);
> > + ABTS_ASSERT(tc, "returned events incorrect",
> > + ((descs[0].rtnevents | descs[1].rtnevents)
> > + == (APR_POLLIN | APR_POLLOUT))
> > + && descs[0].rtnevents != descs[1].rtnevents);
>
> This fails because both rtnevents are actually APR_POLLIN | APR_POLLOUT.
>
> > + }
> > + else {
> > + ABTS_ASSERT(tc, "either one or two events returned",
> > + lrv == 1 || lrv == 2);
> > + }
> >
> > recv_msg(s, 0, p, tc);
> >
>
> If we get past that point, we later fail because at the line:
>
> ABTS_INT_EQUAL(tc, APR_POLLOUT, descs[0].rtnevents);
>
> The rtnevents is again APR_POLLIN | APR_POLLOUT. Not clear why that's
> the case, but there we have it...
Figured it out. See r386154 for the fix. Long standing bug in the
kqueue code. Will backport to 1.2.x real soon now.
-garrett
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 3/15/06, Joe Orton <jo...@redhat.com> wrote:
> Returning two separate descriptors should be valid behaviour, the test
> just needs to be adjusted to handle that case; it'll still catch the bug
> in the select() backend. Can you try this patch?
Tried on FreeBSD:
> Index: testpoll.c
> ===================================================================
> --- testpoll.c (revision 385518)
> +++ testpoll.c (working copy)
> @@ -309,10 +309,25 @@
>
> rv = apr_pollset_poll(pollset, 0, &lrv, &descs);
> ABTS_INT_EQUAL(tc, 0, APR_STATUS_IS_TIMEUP(rv));
> - ABTS_INT_EQUAL(tc, 1, lrv);
> - ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> - ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
> - ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> + if (lrv == 1) {
> + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> + ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
> + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> + }
> + else if (lrv == 2) {
> + ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
> + ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
> + ABTS_PTR_EQUAL(tc, s[0], descs[1].desc.s);
> + ABTS_PTR_EQUAL(tc, s[0], descs[1].client_data);
> + ABTS_ASSERT(tc, "returned events incorrect",
> + ((descs[0].rtnevents | descs[1].rtnevents)
> + == (APR_POLLIN | APR_POLLOUT))
> + && descs[0].rtnevents != descs[1].rtnevents);
This fails because both rtnevents are actually APR_POLLIN | APR_POLLOUT.
> + }
> + else {
> + ABTS_ASSERT(tc, "either one or two events returned",
> + lrv == 1 || lrv == 2);
> + }
>
> recv_msg(s, 0, p, tc);
>
If we get past that point, we later fail because at the line:
ABTS_INT_EQUAL(tc, APR_POLLOUT, descs[0].rtnevents);
The rtnevents is again APR_POLLIN | APR_POLLOUT. Not clear why that's
the case, but there we have it...
-garrett
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 3/15/06, Joe Orton <jo...@redhat.com> wrote:
> On Wed, Mar 15, 2006 at 12:09:06PM +0000, Joe Orton wrote:
> > On Tue, Mar 14, 2006 at 01:19:31PM -0800, Garrett Rooney wrote:
> > > Does anyone have an objection to reverting the addition of the new
> > > multi_event_pollset test on the 1.2.x branch? It's testing for
> > > behavior that's never existed for a number of pollset back ends (at
> > > least kqueue, potentially epoll as well), and nobody has jumped up to
> > > make the test pass on those platforms in trunk, let alone get a fix
> > > that can be backported.
> > >
> > > For the curious, the problem is that kqueue doesn't consolidate the
> > > events it returns, so you can get two separate events for the same
> > > file descriptor, some read, some write, etc.
> > >
> > > Given that we've always had this behavior in the 1.x releases and
> > > nobody's complained about it, I'm willing to just say that the test is
> > > bogus. If someone wants to rework the kqueue (and probably epoll)
> > > impls to meet this new standard that's something that can be
> > > discussed, but for now there's no real justification for us to
> > > suddenly condemn their behavior.
> >
> > Returning two separate descriptors should be valid behaviour, the test
> > just needs to be adjusted to handle that case; it'll still catch the bug
> > in the select() backend. Can you try this patch?
>
> Ah, sorry, I see Brian had already posted an almost identical patch.
> With either this or his patch applied, the test looks correct to me.
> After the recv_msg call there should not be available data to read from
> the socket, so if rtnevents has POLLIN set at that point, it sounds like
> a real bug.
I can't recall if I tested Brian's old patch on FreeBSD or Darwin.
It's quite possible I tested on Darwin, which as we now know has some
bustedness in its kqueue support. I'll try and give it a shot on
something with a working kqueue today.
-garrett
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Mar 15, 2006 at 12:09:06PM +0000, Joe Orton wrote:
> On Tue, Mar 14, 2006 at 01:19:31PM -0800, Garrett Rooney wrote:
> > Does anyone have an objection to reverting the addition of the new
> > multi_event_pollset test on the 1.2.x branch? It's testing for
> > behavior that's never existed for a number of pollset back ends (at
> > least kqueue, potentially epoll as well), and nobody has jumped up to
> > make the test pass on those platforms in trunk, let alone get a fix
> > that can be backported.
> >
> > For the curious, the problem is that kqueue doesn't consolidate the
> > events it returns, so you can get two separate events for the same
> > file descriptor, some read, some write, etc.
> >
> > Given that we've always had this behavior in the 1.x releases and
> > nobody's complained about it, I'm willing to just say that the test is
> > bogus. If someone wants to rework the kqueue (and probably epoll)
> > impls to meet this new standard that's something that can be
> > discussed, but for now there's no real justification for us to
> > suddenly condemn their behavior.
>
> Returning two separate descriptors should be valid behaviour, the test
> just needs to be adjusted to handle that case; it'll still catch the bug
> in the select() backend. Can you try this patch?
Ah, sorry, I see Brian had already posted an almost identical patch.
With either this or his patch applied, the test looks correct to me.
After the recv_msg call there should not be available data to read from
the socket, so if rtnevents has POLLIN set at that point, it sounds like
a real bug.
joe
Re: testpoll.c:multi_event_pollset and 1.2.x
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Mar 14, 2006 at 01:19:31PM -0800, Garrett Rooney wrote:
> Does anyone have an objection to reverting the addition of the new
> multi_event_pollset test on the 1.2.x branch? It's testing for
> behavior that's never existed for a number of pollset back ends (at
> least kqueue, potentially epoll as well), and nobody has jumped up to
> make the test pass on those platforms in trunk, let alone get a fix
> that can be backported.
>
> For the curious, the problem is that kqueue doesn't consolidate the
> events it returns, so you can get two separate events for the same
> file descriptor, some read, some write, etc.
>
> Given that we've always had this behavior in the 1.x releases and
> nobody's complained about it, I'm willing to just say that the test is
> bogus. If someone wants to rework the kqueue (and probably epoll)
> impls to meet this new standard that's something that can be
> discussed, but for now there's no real justification for us to
> suddenly condemn their behavior.
Returning two separate descriptors should be valid behaviour, the test
just needs to be adjusted to handle that case; it'll still catch the bug
in the select() backend. Can you try this patch?
Index: testpoll.c
===================================================================
--- testpoll.c (revision 385518)
+++ testpoll.c (working copy)
@@ -309,10 +309,25 @@
rv = apr_pollset_poll(pollset, 0, &lrv, &descs);
ABTS_INT_EQUAL(tc, 0, APR_STATUS_IS_TIMEUP(rv));
- ABTS_INT_EQUAL(tc, 1, lrv);
- ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
- ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
- ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
+ if (lrv == 1) {
+ ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
+ ABTS_INT_EQUAL(tc, APR_POLLIN | APR_POLLOUT, descs[0].rtnevents);
+ ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
+ }
+ else if (lrv == 2) {
+ ABTS_PTR_EQUAL(tc, s[0], descs[0].desc.s);
+ ABTS_PTR_EQUAL(tc, s[0], descs[0].client_data);
+ ABTS_PTR_EQUAL(tc, s[0], descs[1].desc.s);
+ ABTS_PTR_EQUAL(tc, s[0], descs[1].client_data);
+ ABTS_ASSERT(tc, "returned events incorrect",
+ ((descs[0].rtnevents | descs[1].rtnevents)
+ == (APR_POLLIN | APR_POLLOUT))
+ && descs[0].rtnevents != descs[1].rtnevents);
+ }
+ else {
+ ABTS_ASSERT(tc, "either one or two events returned",
+ lrv == 1 || lrv == 2);
+ }
recv_msg(s, 0, p, tc);