Phantom event callbacks with EV_ET

[htuch]

In Envoy today, we sometimes have network socket FDs setup with event_assign to watch for EV_CLOSED | EV_ET | EV_PERSIST. The callback function is invoked on EV_READ or EV_WRITE however, as a result of the following call chain:

1. epoll_dispatch issuing read/write events with EV_ET set, see

   libevent/epoll.c

   Line 504 in 0d2f170

   evmap_io_active_(base, events[i].data.fd, ev | EV_ET);

   .

2. evmap_io_active masking out EV_READ and EV_WRITE (since we're only looking for EV_CLOSED) but preserving EV_ET in

   libevent/evmap.c

   Line 436 in 0d2f170

   event_active_nolock_(ev, ev->ev_events & events, 1);

   .

3. event_active_nolock_ delivering an event to the callback with res set to EV_ET, despite no registered event actually firing.

This can be fixed with the following patch:

diff --git a/evmap.c b/evmap.c
index ffc991f5..d824c062 100644
--- a/evmap.c
+++ b/evmap.c
@@ -432,7 +432,7 @@ evmap_io_active_(struct event_base *base, evutil_socket_t fd, short events)
        if (NULL == ctx)
                return;
        LIST_FOREACH(ev, &ctx->events, ev_io_next) {
-               if (ev->ev_events & events)
+               if (ev->ev_events & (events & (EV_READ | EV_WRITE | EV_CLOSED)))
                        event_active_nolock_(ev, ev->ev_events & events, 1);
        }
 }

I'm wondering if folks agree that this is a bug and whether the above patch is the correct fix?

CC @asraa @adisuissa

[azat]

But then you will miss the close event, no? (FWIW if you will provide a 50L reproducer that will helps in understanding what you are trying to solve, looks that it is pretty simple to write such)

[htuch]

@azat the above patch explicitly allows for EV_CLOSED in the whitelist of events that we mask in. I can work on a reproducer, sure.

[htuch]

@azat see htuch/envoy@0463f3b for a minimal reproducer. You can see here that we're subscribed only for EV_CLOSED events, but an injected EV_READ triggers the callback.

[azat]

event_active_nolock_ delivering an event to the callback with res set to EV_ET despite no registered event actually firing.

But you are calling event_active() which makes an event active manually (so not sure how this is related to the evmap and epoll code), and it will make it active regardless it was registered with this events or not

P.S. your patch will break real EV_CLOSED handling

[htuch]

I'll work on an FD-triggered example. Can you explain how this breaks real EV_CLOSED handling? AFAICT, I'm just masking READ/WRITE/CLOSED events in for consideration, and ignoring other things like EV_ET.

[azat]

Indeed, but EV_CLOSED will be delivered with EV_READ|EV_WRITE (i.e. EOF) instead, which will be filtered out before and the callback will never be triggered and you will got endless loop - repro.