Dedicated event for EPOLLHUP, EPOLLERR

[bschlinker]

I'd like to add another event type that enables notification of EPOLLHUP and EPOLLERR without requiring a subscription to EV_READ or EV_WRITE.

I've done a rough cut of this and would like to upstream.

Why this is useful:

• EPOLLHUP and EPOLLERR are currently delivered as (EV_READ | EV_WRITE)
• In some cases, the application may not be subscribed for EV_READ or EV_WRITE, but still want to be notified of EPOLLHUP / EPOLLERR:
  • For instance, a server in the process of closing a connection may unsubscribe from EV_READ messages to put back pressure on the transport and may not be subscribed to EV_WRITE as there are no writes pending.
  • However, the server may want to be notified of connection failures, or of events such as socket timestamps being delivered via the socket error queue.

Questions:

• Two new events or one: At the moment I've added a single event, EV_HUPERR, that captures EPOLLHUP and EPOLLERR. I could split into two separate events, but I think most use cases will want both. Looking for feedback on whether flexibility should be prioritized
• Name of the event: Can be EV_HUPERR, or EV_ERRHUP, or anything else. There was some relevant discussion in libevent hides POLLERR #495

Seems related to #345; using a new issue since that one is 4 years old.

[azat]

Hi, good question, but let's look at the existing bits.

(all above about epoll)

For instance, a server in the process of closing a connection may unsubscribe from EV_READ messages to put back pressure on the transport and may not be subscribed to EV_WRITE as there are no writes pending.

EV_CLOSE (EPOLLRDHUP) not the same?

However, the server may want to be notified of connection failures, or of events such as socket timestamps being delivered via the socket error queue.

And EPOLLERR is always monitored (you don't need to specify it, so it will be monitored explicitly with EPOLLRDHUP)

[bschlinker]

Thanks for the reply -- sorry for the late follow up from my side.

EV_CLOSE (EPOLLRDHUP) not the same?

• My understanding is that a socket can close without a EPOLLRDHUP event occurring. There's some useful discussion in on this in a StackOverflow answer ([1]), which matches my review of the kernel.
• Messages in the socket error queue result in a EPOLLERR message ([2]).
• Socket errors (such as a RST being received), also result in EPOLLERR ([3], [4], [5]).

Based on this, I don't think EPOLLRDHUP is the right signal.

And EPOLLERR is always monitored (you don't need to specify it, so it will be monitored explicitly with EPOLLRDHUP)

I understand that EPOLLERR and EPOLLHUP are always added to the list of monitored events in eventpoll.c ([6]) and understand the idea of subscribing to EV_CLOSED events to get these signals.

However, I don't think this approach will work given the current implementation:

• If only EV_CLOSED is subscribed to, epoll_dispatch will set EV_READ and EV_WRITE when EPOLLERR and/or EPOLLHUP signals are raised ([7]).
• However, no event will get to the application because EV_READ and EV_WRITE are not subscribed to, and thus the check in evmap_io_active_ on ev->ev_events will fail ([8], [9]).

Thus, it appears if only the EV_CLOSED event is subscribed to the application does not get notified of events in the socket error queue, or socket errors (such as resets).

---

Based on the above, there doesn't seem to be a way to listen for EPOLLERR or EPOLLHUP messages unless read and write events are also listened to -- and the application may not want to this if it intentionally wants to put back pressure on the transport and has nothing to write.

This can be resolved by either setting the EV_CLOSED event when there is a EPOLLERR or EPOLLHUP pending (as is done for EV_READ and EV_WRITE ([10])), or by adding a new event. Let me know if I've misunderstood something during my trace.

[bschlinker]

@azat Assuming I haven't misunderstood something during my trace, I think there's two options:

1. Modify the EV_CLOSED event to also be set when there is a EPOLLERR or EPOLLHUP event.
2. Create one or two new events that are explicitly for EPOLLERR and EPOLLHUP.

Would be great to get your feedback as I'm adding socket timestamp support to a socket library that is also open source, and want to ensure that the change is upstream compatible.

One concern about option (1) is possible side effects on existing use cases. In addition, dependencies, such as the open source socket library I'm modifying, will need to be able to determine whether the libevent version includes the modification or they won't be able to operate safely.

[azat]

@bschlinker hi!
Sorry for the delay and many thanks for such verbose descriptions!

I've done a rough cut of this and would like to upstream.

Yes, please! (actually I missed this paragraph, sorry)

Does your changes modifies all backends or only epoll?

Based on this, I don't think EPOLLRDHUP is the right signal.

Indeed reasonable

Thus, it appears if only the EV_CLOSED event is subscribed to the application does not get notified of events in the socket error queue, or socket errors (such as resets).

Yes this is tricky (actually comment for it says that it requires EV_FEATURE_EARLY_CLOSE, but who should care about comments, everything should just make sense)

This can be resolved by either setting the EV_CLOSED event when there is a EPOLLERR or EPOLLHUP pending (as is done for EV_READ and EV_WRITE ([10])), or by adding a new event. Let me know if I've misunderstood something during my trace.

Totally agree, let's go with a new event

One concern about option (1) is possible side effects on existing use cases. In addition, dependencies, such as the open source socket library I'm modifying, will need to be able to determine whether the libevent version includes the modification or they won't be able to operate safely.

Yes, any change (especially in the core part) should be backward compatible, so modifying EV_CLOSED is not an option

Create one or two new events that are explicitly for EPOLLERR and EPOLLHUP.

AFAICS it maybe useful, not for subscribing but for distinguishing in the event callback EPOLERR from EPOLLHUP (although he need to always subscribe to both to check hang up on error), for example polling of sysfs will return POLLERR|POLLPRI 1

Also we need to check how this works in other backends, and if the behavior cannot be common for all we can add some feature like EV_FEATURE_EARLY_CLOSE to give the user information about this.

So I would try add two separate events and see how it will looks...

[bschlinker]

Thanks for the detailed response! Sounds like we're on the same page.

Does your changes modifies all backends or only epoll?

Unfortunately I have only done this for the epoll backend -- have never looked through the code for the others. I also did my initial patch on an older version of libevent that has a fork to add event EV_PRI for EPOLLPRI [1]; let me look into whether I need to upstream that as well.

I will start creating EV_ERR and EV_HUP; let me know if you have other preferred names.

[1]: Relevant: https://github.com/facebook/folly/blob/master/folly/io/async/EventHandler.h#L47

[azat]

I will start creating EV_ERR and EV_HUP; let me know if you have other preferred names.

EV_ERR defined in win32 AFAICS, EV_ERROR is not, let use it then

[azat]

@bschlinker by the way do you have any ETA? I'm planning to make a 2.2 alpha release (#1094) and want to include this change too

[nspring]

@azat I started working on this issue and think I have a reasonable cut at it.

This raised two questions:

1. EV_ERROR is defined on mac, but it only warns -- do you have a suggestion for another name? or let it warn?:

  CC       kqueue.lo
In file included from kqueue.c:65:
In file included from ./event-internal.h:41:
In file included from ./minheap-internal.h:33:
./include/event2/event.h:986:9: warning: 'EV_ERROR' macro redefined [-Wmacro-redefined]
#define EV_ERROR        0x200
        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/event.h:170:9: note: previous definition is here
#define EV_ERROR            0x4000      /* error, data contains errno */
        ^

2. epolltable-internal.h is 2.8MB with 5 events, up from 40K. This feels extra large to me, and likely easily compressed by omitting comments or using the preprocessor; do you have a suggestion? Leave it alone? Cut until it reaches a target file size? Just drop the comments and see what happens?

Any other feedback is welcome of course.

[azat]

Sorry for such a huge delay.

EV_ERROR is defined on mac, but it only warns -- do you have a suggestion for another name? or let it warn?:

EV_EVENT maybe ?

epolltable-internal.h is 2.8MB with 5 events, up from 40K. This feels extra large to me, and likely easily compressed by omitting comments or using the preprocessor; do you have a suggestion? Leave it alone? Cut until it reaches a target file size? Just drop the comments and see what happens?

Where do you see 2.8MB?
I think 40K for this generated file is OK, comments could be useful.

P.S. it is generated via make_epoll_table.py