On Jul 24, 7:57pm, <pcg( Marc)@goof(A.).(Lehmann )com> wrote:
> I just found that Event goes into an endless polling loop whenever an error
> occurs on a filehandle:
>
> poll([{fd=3, events=POLLPRI|POLLOUT|POLLWRNORM|POLLRDBAND|POLLWRBAND,
>revents=POLLERR|POLLHUP}], 1, 60000) = 1
> gettimeofday({995996622, 745496}, NULL) = 0
> poll([{fd=3, events=POLLPRI|POLLOUT|POLLWRNORM|POLLRDBAND|POLLWRBAND,
>revents=POLLERR|POLLHUP}], 1, 60000) = 1
> gettimeofday({995996622, 746308}, NULL) = 0
> poll([{fd=3, events=POLLPRI|POLLOUT|POLLWRNORM|POLLRDBAND|POLLWRBAND,
>revents=POLLERR|POLLHUP}], 1, 60000) = 1
> gettimeofday({995996622, 747280}, NULL) = 0
>
> without ever calling a callback.
Urr...
> Reading the source I see that poll => 'e' does not, as I anticipated,
That's frequently what people _think_. Technically, the "exception"
for select is "priority" or "out of band" data.
> get called on error conditions (unless select() is being used)
This varies depending on the system. Many systems have all three of
read, write, and exception reading as "usable" if an error condition
occurs; others just do read and write. (I might mention that, for
instance, FreeBSD, from my reading of the kernel code, likewise
returns all three as "usable" on a POLLHUP condition.)
> but ONLY on high priority read data (interesting side note: 'R' does
> not include high-priority read data but 'W' does include high
> priority write data, which makes for some nice asymetry).
True, although it could be argued that that's what "E" is for. Systems
using select are actually inconsistent in whether they treat "out of
band" or "priority" or "high-priority" data as being both R and E, or
just E. (There is no "high-priority" write data, only "high-priority"
read data, weirdly enough.) Remember, of course, that "E" is only for
being able to _read_ "exceptional" data.
> If an filehandle returns POLLHUP or POLLERR Event goes into an
> endless CPU-eating poll-loop.
Ouch! Yes, the current code would do this... this needs to be fixed,
definitely.
> I temporarily worked around this bug by changing c/unix.c:141 from:
>
> if (mask & (POLLRDBAND | POLLPRI)) got |= PE_E;
>
> to
>
> if (mask & (POLLRDBAND | POLLPRI | POLLERR | POLLHUP))
> got |= PE_E;
>
> however, wether this is the right fixc is the question: when select is
> used, errors are reported using 'e', so it's probably the right thing
> to do. I also think that the asymmetric distribution of POLLWRBAND
> vs. POLLRDBAND is also not what is expected.
My suggested patch is below. With it, R will pick up normal,
"priority", and "high-priority" data readable, plus a POLLHUP or
POLLERR. E will pick up "priority" or "high-priority" data readable,
plus a POLLHUP or POLLERR. W will pick up normal and "priority" data
writeable, plus a POLLERR, and _if_ the event isn't also looking for
"R" or "E", POLLHUP. I'm doing it this way because writing on a closed
socket (as indicated by POLLHUP) generally causes a SIGPIPE, which
will kill the program unless caught or ignored, so it would be
preferable if the program "found out" about the closure of the socket
with an error on reading. The below passes all tests, BTW, at least on
IRIX 6.5.13m, for which poll, not select, is used as people will recall.
diff -NaurdbB -xCVS -xconfigure$ -xconfig.h.in -xMakefile.in Event-0.84.old/c/unix.c
Event-0.84/c/unix.c
--- Event-0.84.old/c/unix.c Tue Jan 22 01:08:05 2002
+++ Event-0.84/c/unix.c Mon Jan 28 18:39:14 2002
@@ -89,8 +89,10 @@
ev->xref = -1;
assert(fd >= 0); {
int bits=0;
- if (ev->poll & PE_R) bits |= (POLLIN | POLLRDNORM);
- if (ev->poll & PE_W) bits |= (POLLOUT |POLLWRNORM |POLLWRBAND);
+ /* POLLIN = POLLRDNORM|POLLRDBAND -Allen */
+ if (ev->poll & PE_R) bits |= (POLLIN | POLLPRI);
+ /* POLLOUT = POLLWRNORM -Allen */
+ if (ev->poll & PE_W) bits |= (POLLOUT | POLLWRBAND);
if (ev->poll & PE_E) bits |= (POLLRDBAND | POLLPRI);
assert(bits); {
int ok=0;;
@@ -131,19 +133,26 @@
if (xref >= 0) {
int got = 0;
int mask = Pollfd[xref].revents;
- if (mask & (POLLIN | POLLRDNORM | POLLHUP)) got |= PE_R;
- if (mask & (POLLOUT | POLLWRNORM | POLLWRBAND)) got |= PE_W;
- if (mask & (POLLRDBAND | POLLPRI)) got |= PE_E;
+ if (mask & (POLLIN | POLLPRI | POLLHUP | POLLERR)) got |= PE_R;
+ if (mask & (POLLOUT | POLLWRBAND | POLLERR)) got |= PE_W;
+ if (mask & (POLLRDBAND | POLLPRI | POLLHUP | POLLERR)) got |= PE_E;
if (mask & POLLNVAL) {
warn("Event: '%s' was unexpectedly closed",
SvPV(ev->base.desc, n_a));
pe_io_reset_handle((pe_watcher*) ev);
+ } else {
+ if ((mask & POLLHUP) && (ev->poll & PE_W) && (!(got & PE_W))
+ && (!(ev->poll & PE_R)) && (!(ev->poll & PE_E))) {
+ /* Must notify about POLLHUP _some_ way - Allen */
+ got |= PE_W;
}
- else if (got) _queue_io(ev, got);
+
+ if (got) _queue_io(ev, got);
/*
Can only do this if fd-to-watcher is 1-to-1
if (--ret == 0) { ev=0; continue; }
*/
+ }
}
ev = next_ev;
}
--
Allen Smith [EMAIL PROTECTED]
September 11, 2001 A Day That Shall Live In Infamy II
"They that can give up essential liberty to obtain a little temporary
safety deserve neither liberty nor safety." - Benjamin Franklin
